Skip to content

Conversation

@jecisc
Copy link
Member

@jecisc jecisc commented Apr 4, 2023

#willJumpIfFalse is already a method present un Debbuging-Core in Pharo.

This method creates a clash with the one already in Pharo and need to be removed.

@jecisc jecisc changed the title Update InstructionStream.extension.st Remove #willJumpIfFalse Apr 4, 2023
@jecisc
Copy link
Member Author

jecisc commented Apr 6, 2023

The error does not seems related to my PR

@adri09070
Copy link
Collaborator

Was it recently added to Debugging-Core? Because, if Pharo let me create it, it means it didn't exist in Debugger-Core, right?

@jecisc
Copy link
Member Author

jecisc commented May 10, 2023

It has been there for a while I think. At least some months.

Pharo does not prevent to override existing methods, that’s what is happening each time we update a method. So extensions can replace existing methods but we end up in a dirty state

@StevenCostiou
Copy link
Member

How did you detect the clash?
Because I can only see the Sindarin protocol and not the original method.

There is only one usage, in the Flashback Decompiler.
I did not know that package.

@StevenCostiou
Copy link
Member

Errors are unrelated but are scary: "could not resolve baseline of Sindarin"?

@adri09070
Copy link
Collaborator

Errors are unrelated but are scary: "could not resolve baseline of Sindarin"?

it's ok, it will be resolved once #61 is merged, I think

@jecisc
Copy link
Member Author

jecisc commented May 10, 2023

How did you detect the clash? Because I can only see the Sindarin protocol and not the original method.

There is only one usage, in the Flashback Decompiler. I did not know that package.

I detected the clash while doing a "recalculate dirty packages" on the Pharo repository. We currently have two clashes and this one is one of them.

Here is the version in the Pharo repository: https://github.com/pharo-project/pharo/blob/Pharo12/src/Debugging-Core/InstructionStream.extension.st#L130

I prioritized keeping the one that is in Debbugging-Core but if you think there is a better solution I'm also open to it. I'd just like to avoid clashes of methods in the base image of Pharo

@StevenCostiou
Copy link
Member

No you are right, the original one should be kept.
I just did not know how to detect that kind of case.

I will fix the baseline problem then merge this PR.

@StevenCostiou
Copy link
Member

eeeh just a question: that is going to Pharo 11 ?

@adri09070 adri09070 closed this May 10, 2023
@adri09070 adri09070 reopened this May 10, 2023
@adri09070
Copy link
Collaborator

Well, tests don't pass because of the removed method. It doesn't find the other one

@jecisc
Copy link
Member Author

jecisc commented May 10, 2023

It should not break P11 I think but if we can keep it for p12 it would be nice since the release is for this week :)

@jecisc
Copy link
Member Author

jecisc commented May 10, 2023

Well, tests don't pass because of the removed method. It doesn't find the other one

Hum... This is weird because it's present in Pharo (cf link I provided just before)

@StevenCostiou
Copy link
Member

I think the other PR I just merged went to P11 :/
How do I make it go to P12?

@adri09070
Copy link
Collaborator

adri09070 commented May 10, 2023

I think the other PR I just merged went to P11 :/
How do I make it go to P12?

No, it's not. It has been merged into master.
To merge it into Pharo11, we need to merge master into the Pharo-11 branch.

To merge it into Pharo12, we should create a branch Pharo-12, merge master into Pharo-12, and change the baseline of NewTools to load the Pharo12 branch

@StevenCostiou
Copy link
Member

Ah! Nice, so no impact? We can continue merging on master?

@adri09070
Copy link
Collaborator

Ah! Nice, so no impact? We can continue merging on master?

yes we can

@jecisc
Copy link
Member Author

jecisc commented May 17, 2023

My guess is that the problem is this:

  • The Pharo image get bootstraped and loads the Debugging-Core package with the Pharo version of #willJumpIfFalse
  • Later in the bootstrap Sindarin is loaded and install its own version of #willJumpIfFalse
  • The CI here uses this image and unload Sindarin to reload the new version, its version of #willJumpIfFalse is removed but the Pharo original version is not installed back.

So, merging this should be fine.

@adri09070
Copy link
Collaborator

It looks correct.

What I can do is merge this into the master branch as it has no direct effect on Pharo. (Pharo11 and 12 loads the Pharo-11 branch).

And then I can run another CI on the master branch and it should be green

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants