Metacello / metacello

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

Squeak: introduce a new MCFilesystemFetchOnlyRepository #515

Closed tom95 closed 4 years ago

tom95 commented 4 years ago

Rationale: the current MCGitBasedNetworkRepository conflates downloading sources with deserializing them, making it harder to support non-filetree-based formats (e.g. tonel).

Proposal: For now only on the Squeak platform, we introduce a new MCFilesystemFetchOnlyRepository that defers deserialization to either the MCFileTreeRepository or the TonelRepository (from @j4yk's fork that supports Squeak, as we're scoping these changes only to the Squeak platform package at the moment).

The current implementation is essentially just a copy of the MCGitBasedNetworkRepository subclass tree, with all (I hope it's all) deserialization calls forwarded to a repositoryReader that is determined based on the repo contents (filetree or tonel). If tonel is not installed, we present a dialog to allow the user to download it.

I've mainly decided to limit this to Squeak because Pharo is already rolling a similar solution, so they won't have a need for this, and I don't have a very good overview on how other Smalltalk flavors are using Metacello right now.

For the implementation, naming, and the usage workflow, I'd be very interested in your feedback. I got a lot of hints from Jakob (@j4yk) already, but I'm mainly opening this PR right now so everyone can take a detailed look at the implementation proposal.

Since there is a lot of copied code, here's a list of things that may be of larger interest:

Open Problems:

How to Test: In a fresh Squeak image, run:

Metacello new
    baseline: 'Metacello';
    repository: 'github://tom95/metacello:squeak-download-repo-split/repository';
    get; load: #default.
Metacello new
        repository: 'github://PolyMathOrg/PolyMath/src';
        baseline: 'PolyMath';
        load

Rejoice that PolyMath started loading and only started failing once classes were encountered that are Pharo-only!

dalehenrich commented 4 years ago

@tom95, Well it seems that the squeak builds are almost all failing on travis, and the squeak builds have been expected failures for a very long time, because noone (or very few developers) in the Squeak community cared enough to correct the travis build issues ... The one squeak build I looked at is failing due to #514 ... which is a consequence of recent changes to the github api ... gemstone has the same issue and I don't have the cycles to fix the issue for gemstone let alone squeak...

Soooo, with failing tests and a lack of cycles on my part, I'd be willing to press the merge merge button if you are confident that your changes aren't the source of the failures ... even better would be a cleanup of the squeak failing tests so that squeak builds can be moved from expected failures ...

@krono is a member of the Metacello project, so I think he should also have the ability to press the merge button on Squeak's behalf (if he doesn't I will give him permission).

@tom95, take a look at the travis failures and make sure they are not side effects of your changes and let me know if you want me to press the merge button ...

marceltaeumel commented 4 years ago

Hmmm... I just looked at those failing CI builds for Squeak. It seems quite strange that the requests behind MCGitHubRepository>>downloadJSONTags seem to time out during an HTTP request ... Works fine in my local images.

The other issues are behind MetacelloSqueakPlatform>>downloadZipArchive:to:. If I recall correctly, those have been there for ages. And they do work fine locally. Very strange. Those downloads seem to be faulty.

Regarding those other failing tests, SmalltalkImage>>removeKey:ifAbsent: might have to be added as a Metacello extension again. Or the tests changed from "Smalltalk removeKey..." to "Smalltalk globals removeKey..." or "Environment default removeKey...".

krono commented 4 years ago

Hi!

@krono is a member of the Metacello project, so I think he should also have the ability to press the merge button on Squeak's behalf (if he doesn't I will give him permission).

I have :)

Also, we have to see wether the problems are bootstrap problems. I kind of remeber that the download zip stuff is different in the bootstrap compared to the actual current version…

dalehenrich commented 4 years ago

@marceltaeumel and @krono ... I think it's worth the effort to get the tests passing on travis and yes it is often more difficult to get test to pass on travis ... hopefully we can get to the point where we expect the tests to pass ... and I hope to find the time to resolve the new github api issues for gemstone:)

@marceltaeumel the download json tags issue may be due to the fact that github has changed their api for tags --- this is the source of the recent gemstone failures as well ... the other issue that tends to hit travis ci tests harder than local tests is that the api rate limit for github can be exceeded by other project's tests running behind the same ip address

tom95 commented 4 years ago

I was not able to figure out when failures started occurring (tried going back a couple versions with both WebClient and SqueakSSL), but this turned out to be a fix for the timeout when fetching git tags in my Squeak Trunk image: http://forum.world.st/The-Inbox-WebClient-Core-tobe-120-mcz-td5116209.html

tom95 commented 4 years ago

A quick update on this PR: the tests for Squeak are now passing and we wrote to the squeak-dev mailing list with a request to help test the PR.

In the meantime, I uncovered an issue that may require input from the iceberg devs: This baseline for example is stating that GToolkitBoxer can be loaded from the /src directory, however the code is actually found in /boxer-bindings, which the .project file is also stating. My gut feeling would have been that the explicit URL overrides the .project file and thus allows users to go for a specific directory, however the implementation in iceberg appears to do it the other way around.

dalehenrich commented 4 years ago

@tom95, excellent work and thanks for the update ... @estebanlm, could you take a look at @tom95 question

krono commented 4 years ago

Looks good to me :)

krono commented 4 years ago

🎉