-
-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add local registry with most popular crates from crates.io #27
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks reasonable to me, thank you! Note: I have not yet attempted this locally.
Assuming this also looks good to @iHiD, I'll test it thoroughly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice way to approach this! Do you have any indication on the size increase of the Docker image this PR adds?
exit | ||
fi | ||
else | ||
if [ ! -e Cargo.lock ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if at some later point there would be an error messageif the student uses a non-supported crate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was thinking about this. I'll think of a way to support this.
Dockerfile
Outdated
WORKDIR /local-registry | ||
COPY local-registry/* ./ | ||
RUN curl "https://crates.io/api/v1/crates?page=1&per_page=100&sort=downloads" | \ | ||
jq -r '.crates[] | .id + "=\"" + .max_stable_version + "\""' >> Cargo.toml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How big are crates? Is it worth doing 1000 instead of 100? Or are they big?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the top 100 crates it's just 17MB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be up for expanding it to the top 500 then? @ErikSchierboom?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷 It all depends on how likely it is those crates are used. I have no data to back up any claims though. @coriolinus @dhovart how likely is it that students would used a crate that is not in the top 100 crates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have absolutely no idea, sorry! I just know that this list should maybe be fine-tuned. There are crates we pull that make no sense in the context of exercism, such as for gui development.
Maybe there should be a message for students somewhere (below the list of supported crates maybe) that say that if they want support for a given crate, they should open a GH issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of the crates.io API, we would get the list from somewhere else, where it was edited.
I guess I can add an endpoint to exercism's site API. But I'm not familiar with the site codebase yet, is there some administration interface where someone could create this list ? (if my proposal makes sense)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have absolutely no idea, sorry! I just know that this list should maybe be fine-tuned.
I think having a curated list makes the most sense. We definitely don't need to have any GUI crates included, and that would also allow us to cherry-pick crates that are not in the top 100, but still useful for students in the context of Exercism.
Instead of the crates.io API, we would get the list from somewhere else, where it was edited.
We haven't yet come up with a spec for this. I think we want this list to be in the track repo at exercism/rust. Having it there means that we'll be able to programmatically access it from the website. The next question then becomes: where to store it? I think there are basically two options:
- Add a section to the root
config.json
file - Add a new file just to list the supported libraries
I don't yet have a real preference, but the first option might be best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be fine as MVP to get it just as a file in this repo that's read when Docker builds. That should be quite trivial. Then we can improve it later. In my eyes this is a real priority, so the quickest solution is probably the best one! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be fine as MVP to get it just as a file in this repo that's read when Docker builds.
I wouldn't want to touch the existing Docker push workflow (we want to sync that across all repos at some point) and secondly, I'd prefer it to be in exercism/rust as there will be more visibility with maintainers (and students will figure out that location sooner than they will this repo).
@dhovart Brilliant. Thanks! I'll leave @ErikSchierboom to help get it over the line from a Docker perspective, and @coriolinus to test it all out, but theoretically seems great to me! :) |
I think we need to let users know which crates are available. Is there a way we can reliably do that as the "top x crates" might change between deployment and someone looking at the website if we do ad hoc lookups? Could the GHA write a file when it builds the docker with the list of crates, which gets read by the website. Thoughts? |
I think so. Might have to open a PR instead of writing directly to a file, but such a thing should be possible. |
|
The above comment is an analysis of all the dependencies that were used and their counts and versions. |
I've made a small change to generate the registry from the list of crates above, using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if you could add some tests to verify that exercises that use one of these crates work fine.
Looking at this repo, AFAICT the existing test are all unit tests. While useful, I'd encourage test runner repos to also have some golden (integration) tests, where you have exercises at test data along with an expected results.json file. Running the test runner on those exercises should then produce the exact same results.json file.
For an example of how this works, see:
- https://github.com/exercism/prolog-test-runner/tree/main/tests/example-empty-file
- https://github.com/exercism/prolog-test-runner/tree/main/tests
- https://github.com/exercism/prolog-test-runner/blob/main/bin/run-tests.sh
- https://github.com/exercism/prolog-test-runner/blob/main/bin/run-tests-in-docker.sh
I'd be totally fine with adding tests in a follow-up PR.
Apart from that, this PR looks good to me. I'll leave it up to Rust maintainers (@coriolinus) to confirm that they're happy with this PR.
Great work!
14fe00a
to
bce6303
Compare
bce6303
to
74629fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still haven't actually tested this, sorry. Lots going on all at once on my end.
From a Rust perspective this looks both straightforward and acceptable. I just want someone on the maintainer side to run through it once locally, and the time in which I expected to do that myself hasn't materialized. Sorry.
I've tried running this on the exemplar solution for the The command I've run is:
This results in the following error:
It works fine for |
I did try with gigasecond (before my most recent changes) and it worked. I'll run some more tests later today or tomorrow if I can find the time. |
@dhovart Ah, that might have been the issue then. I don't think the exemplar implementation has one. I'll check tomorrow. |
@dhovart I've found the issue. This solution doesn't work with a read-only file system. That's not an issue at this moment, as we don't yet run on a read-only file system. We might at some point want to enable this, but we can safely ignore this for now. This is the output BTW:
I'm noticing that the dependencies seem to be compiled before being able to run the tests. Is there any way in which we can pre-compile all the packages? |
Thanks a ton @dhovart! |
You're welcome! I'll check if there's a way to use precompiled libraries. |
This is great. (This comment is just here so I can find it back using GitHub's history, since we probably want to do this for other languages too) |
I was thinking that one way to generalize this for different languages would be to have packages managed by Nix, but I'm no nix expert. Maybe something to look into at some later date if someone is willing to. |
This fixes exercism/rust#1307
This will download to a local registry the top 100 most downloaded crates from crates.io
To regenerate the registry, specify
/opt/test-runner/bin/regenerate-registry.sh"
as the entrypoint.