search

Metacello / metacello

Metacello is a package management system for Smalltalk
MIT License
87 stars 43 forks source link

Recent release of Metacello master has broken 3.5.x GemStone builds #518

Closed dalehenrich closed 4 years ago

dalehenrich commented 4 years ago

The expected failures for GemStone builds and metacello involved failing JSON tests, as reported #514, not Security errors while installing SmalltalkCI for 3.5.x builds that we are now seeing in across the board travis ci builds.

These particular errors are showing up because normal GemStone builds use https://github.com/dalehenrich/metacello-work, however, SmalltalkCi itself ends up 'unconditionally' loading https://github.com/Metacello/metacello (hpi-swa/smalltalkCI/#459)

dalehenrich commented 4 years ago

@tom95 and @krono, I have finally gotten around to fixing the JSOn issues that were causing massive failures (#514) and I'm in the process of merging my fixes into Metacello/metacello and I have noticed that there are still some build failures for GemStone 3.5.x.

Apparently these failures weren't noticed when you were doing your work, because of the extra noise created by the failing JSON tests...

I've tracked it down to the fact that the setLoadsInMetacelloProject: for Collection and String have been moved form the Metacello-MC package to the Metacello-Core package ... at the end of the day this is actually a bug in the 3.5.x releases, but this is released code and I cannot fix them retroactively....

Sooooo, I am wondering why the change was made? It seems that the Metacello-MC package is being loaded into Squeak, so I'm not sure that these methods have to be in Metacello-Core.

On the issue_514 branch I will be making changes to the packaging to see if can get Metacello/metacello loading into the 3.5.x releases ...

Note that Metacello/metacello is not the primary repository for GemStone (dalehenrich/metacello-work is the primary), but I was planning on moving to Metacello/metacello as the primary and this issue if unaddressed will prevent me from making the move for a while longer.

dalehenrich commented 4 years ago

@tom95 and @krono, it looks like moving those methods back into Metacello-MC had no obvious adverse affects on the overall builds. I'm moving Pharo-6.0 to expected failures, since the build just plain fails now ... There ARE new failures {?} for Squeak64-trunk ... I'll hold off merging my GemStone changes into master until you guys have a chance to take a look at those failures.

tom95 commented 4 years ago

@dalehenrich thank you for alerting us! It appears that reading the source code during that test fails in some way (the temporaries are replaced by the characteristic t1, t2 names of the decompile string). I'll see if I can figure out how that comes to be.

tom95 commented 4 years ago

This is the breaking change: http://forum.world.st/The-Trunk-Monticello-jr-722-mcz-td5118088.html

We no longer log the source of the added methods into the sources file, so trying to read them back only yields the decompile string. The MetacelloToolBox, however, of course produces a proper source string. When comparing the stored version version and the newly constructed version, we thus fail because the argument is called t1 in the stored version and spec in the newly created.

So, since MetacelloDictionaryRepositoryTest>>#runCase uses #suspendSystemUpdateEventsDuring:, we no longer get the originally compiled string back. I verified that changing asMethodAddition to always logSource again allows all tests to pass. Now, my knowledge of the exact impact of that change is rather limited, as such I would tag @j4yk, the author of Monticello-jr.722.

I suppose a simple fix in Metacello, rather than Squeak, would be to always produce a decompile string from the source string MetacelloToolBox creates, but maybe there's a better way.

tom95 commented 4 years ago

Ah I may want to add, while the failing build may not look nice, Squeak-trunk being unstable and these failures only affecting the testing environment, I would say you could move ahead with merging the changes @dalehenrich and we try to figure out how to fix the trunk build failure later if you like.

dalehenrich commented 4 years ago

@tom95, I'm fine with that and will go ahead and merge my work ...

j4yk commented 4 years ago

So, since MetacelloDictionaryRepositoryTest>>#runCase uses #suspendSystemUpdateEventsDuring:, we no longer get the originally compiled string back. I verified that changing asMethodAddition to always logSource again allows all tests to pass. Now, my knowledge of the exact impact of that change is rather limited, as such I would tag @j4yk, the author of Monticello-jr.722.

When you revert the change in asMethodAddition, all tests that load something with Monticello will spam the changes file again with no way to silence them.

I don't know the test in question and what it does. Can the decompiled source string be fixed with a bunch of copyReplaceAll:with:? Or instead of the source strings, compare the parse trees or the behavior of the method in question? Yet another idea would be to use suspendSystemUpdateEventsDuring: in a narrower scope.

dalehenrich commented 4 years ago

@tom95, we've reached the point, where the travis-ci bug is fixed and we're down to only the squeak trunk failure keeping us from luscious green:). With that said, I'm fine with whatever you choose to do ... wait/expected failure/fix ...

tom95 commented 4 years ago

Great to hear we'll soon be back to all-green!

I'll just tag the proposal for the fix on Squeak-trunk here for future reference: https://github.com/Metacello/metacello/pull/522 :)