-
Notifications
You must be signed in to change notification settings - Fork 103
match against environment manifest version before using the default channel #1315
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
base: main
Are you sure you want to change the base?
Conversation
src/bin/julialauncher.rs
Outdated
|
|
||
| // Search for manifest files in priority order | ||
| // JuliaManifest.toml takes precedence over Manifest.toml | ||
| for manifest_name in ["JuliaManifest.toml", "Manifest.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.
Needs support for {Julia,}Manifest-vX.Y.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.
One idea:
- Loop over all files in the directory. Ignore folders.
- Collect only the filenames that match the allowed patterns.
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 also be reasonable (and in my mind a bit less magical) to say that version selection via the manifest only happens from a JuliaManifest.toml or Manifest.toml file.
The {Julia,}Manifest-vX.Y.toml manifests would then only come into play if one used a higher precedent order override.
|
How should this work if the project directory has multiple versioned manifests, e.g. |
|
Probably the highest version? |
2510c05 to
842eafa
Compare
That seems okay. If the user explicitly provides the desired version (e.g. |
Yes, if you look at the priority in the README (which I updated here) it is: So an explicitly provided version has top-priority and will always be used. |
|
BTW I think item 3 should be removed from that list? Redundant with item 5, and IIUC you prefer item 4 over item 5? |
220d16a to
58b6b04
Compare
|
I don't fully understand why it is redundant or why this PR makes it more redundant. |
|
I believe @DilumAluthge was just looking at the list as pasted in the comment here, where there are two 3) items. It is all correct in the PR that modifies the README. |
|
Oops, I must have copy pasted from the diff. |
|
I realize a better way of testing this is (of course) to call julia itself with the same project flag and env variable and get the truth from that. |
|
Just had a thought.. how does this handle manifests from julia master, i.e. 1.14, given that there's no "1.14" channel? I guess there isn't a "1.13" either. |
|
It uses nightly channel if the version is higher than any known version. But maybe it should emit some warning when that happens... |
c846ba4 to
988b305
Compare
|
I can remove tests if you want to... But as I have said, you are free to reopen your PR and we can try them on a bunch of cases. But based on
I thought you didn't plan on getting back to that "any time soon" so I started this. I don't know if four days counts as "any time soon" or if just the fact that someone else started working on it made your desire come back. However, some things that this does that might be different from that branch:
|
Oh, I'm talking about the non-testing code. Taking a slightly closer look, it seems like ~700 LOC compared to ~400 in the original PR, which doesn't seem too bad given the extra functionality you mentioned.
Yea, I wasn't thinking of getting back to this any time soon when I said that. I also have a track record of getting around to things later than I expected, which I try to account for. After making that comment, and given you said that functionality seems needed, I then thought I should just "take a peek" at it that evening to see how bad the merge conflicts really are, and then we talked about how the functionality should work over Slack, and all that moved the work from back-of-mind to front-of-mind. I tend to work on what I'm thinking about. Net result: I've ended up poking at this months earlier than I thought I would. If there is value in me spinning up that branch again, it's much more viable than it was four days ago. However, if Sonnet 4.5 can't take care of the test writing, or for some other reason the work ends up in limbo again, I don't currently have the motivation in this area to keep working on this for a prolonged period. Looking at how hard adding project/nightly/versioned manifest support is, and doing it if it's easy, I am up for (but not much more). |
0a18fe8 to
f0e8600
Compare
|
I rewrote a lot of the implementation here because I realized (while being on the treadmill 🏃♂️) that it made a lot of sense to just directly "transpile" the julia code itself that does the project and manifest file resolving. That should make it easier to review, easier to update and more likely to be correct. As a test of one of the functionality I wanted to use this for I did: which installs the |
b133d52 to
e5e59b0
Compare
e5e59b0 to
8d605db
Compare
…hannel alternative to JuliaLang#1095
8d605db to
320a1b6
Compare
|
I think I am fairly happy with this now. Maybe the number of tests is a bit excessive; I basically added a test as soon as I found something missing or buggy. |
|
Personally I'm always a fan of having lots of tests. |
alternative to #1095
I am not much of a rust programmer so I kind of wrote pseudo code and had Claude Code translate it for me. Should still do some manual cleanups.
In use: