Skip to content
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 preference for version named manifest files #43845

Merged

Conversation

IanButterworth
Copy link
Member

@IanButterworth IanButterworth commented Jan 17, 2022

(Updated to latest)

This makes julia prefer manifest files in the format (Julia)Manifest-v{major}.{minor}.toml over a simple (Julia)Manifest.toml, to allow maintaining multiple manifests for different julia versions concurrently.

A new manifest will still save as (Julia)Manifest.toml (the Julia prefix is used if you have a JuliaProject.toml), so it's up to the user to manually rename the manifest to enable this. So Manifest-v1.11.toml would work for v1.11.

A downside of this approach is adding two file stat operations on-top of the most common scenarios of just having a Manifest.toml or no manifest saved at all, which might be non-negligible on some filesystems. There are design optimizations possible there though (see #43845 (comment))

An alternative is to store multiple julia-versioned manifests within a single Manifest.toml

@jpsamaroo
Copy link
Member

An alternative is to store multiple julia-versioned manifests within a single Manifest.toml

If we need to do a breaking change to the Manifest spec in the future (surely we will), then this would be problematic. It's also often easiest to just manage versions as separate files on the filesystem. For example, if I'm developing my package (which supports Julia 1.7) with 1.8 because I like it more, it doesn't mean I want that 1.8-specific bit of Manifest committed with the rest of the versions in the Manifest when I go to push a change.

@IanButterworth
Copy link
Member Author

Good point.

Also, I don't think this would need backporting to 1.6, you'd just include the 1.6 one as Manifest.toml

@pfitzseb
Copy link
Member

Manifest.1.8.toml/Manifest-1-8.toml look better imho.

base/loading.jl Outdated
Comment on lines 411 to 412
const manifest_names = ("JuliaManifest$(VERSION.major).$(VERSION.minor).toml", "JuliaManifest.toml",
"Manifest$(VERSION.major).$(VERSION.minor).toml", "Manifest.toml")
Copy link
Member Author

@IanButterworth IanButterworth Jan 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This order probably makes more sense

Suggested change
const manifest_names = ("JuliaManifest$(VERSION.major).$(VERSION.minor).toml", "JuliaManifest.toml",
"Manifest$(VERSION.major).$(VERSION.minor).toml", "Manifest.toml")
const manifest_names = ("JuliaManifest$(VERSION.major).$(VERSION.minor).toml",
"Manifest$(VERSION.major).$(VERSION.minor).toml",
"JuliaManifest.toml",
"Manifest.toml")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me. I hope we are not wasting time now with a stat call (aka isfile), since it is substantially faster (and more accurate) to just try to open the file name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IanButterworth I think this is resolved by 6c36a8d, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first issue is resolved, but Jameson's point is worth keeping visible. Currently loading does do the bad thing of isfile testing rather than just opening because it happens in different places.

@DilumAluthge
Copy link
Member

DilumAluthge commented Jan 17, 2022

I prefer the format Manifest.1.8.toml, as I find it to be much easier to read.

I'd also be okay with Manifest-1.8.toml, but I don't love it. Manifest.1.8.toml is my first choice.

I really dislike Manifest1.8.toml or Manifest-1-8.toml.

base/loading.jl Outdated Show resolved Hide resolved
@DilumAluthge
Copy link
Member

In the General registry1, we've already implemented the pattern of Manifest.1.6.toml2, Manifest.1.7.toml3, Manifest.1.8.toml4, etc. And I would really like to keep that same naming convention.

Footnotes

  1. https://github.com/JuliaRegistries/General/tree/master/.ci

  2. https://github.com/JuliaRegistries/General/blob/master/.ci/Manifest.1.6.toml

  3. https://github.com/JuliaRegistries/General/blob/master/.ci/Manifest.1.7.toml

  4. https://github.com/JuliaRegistries/General/blob/master/.ci/Manifest.1.8.toml

@DilumAluthge
Copy link
Member

DilumAluthge commented Jan 21, 2022

If I recall correctly (from the last time I looked at this code), you'll need to make a few small changes to Pkg, but nothing terribly difficult.

@DilumAluthge DilumAluthge added the packages Package management and loading label Jan 21, 2022
@vtjnash
Copy link
Member

vtjnash commented Feb 1, 2022

bump?

@vtjnash vtjnash added the forget me not PRs that one wants to make sure aren't forgotten label Feb 1, 2022
@IanButterworth
Copy link
Member Author

It might be good to get some more Pkg views

@KristofferC @StefanKarpinski @fredrikekre

Perhaps run by triage too?

@DilumAluthge
Copy link
Member

It could also be useful to have a Pkg call.

@IanButterworth IanButterworth force-pushed the ib/manifest_named_versions branch from 6c36a8d to 6c3b200 Compare February 2, 2022 00:20
@IanButterworth IanButterworth added the triage This should be discussed on a triage call label Feb 3, 2022
@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Feb 3, 2022
@IanButterworth
Copy link
Member Author

bumping for Pkg team opinions.

Given there's a little work to do in Pkg if this merges, it would be good to get it decided before the 1.8 freeze

@IanButterworth
Copy link
Member Author

I think it's fair to say from a slack discussion in #pkg-dev that there's some dislike of this approach, and a suggestion by @GunnarFarneback to add a --manifest option to julia to point to a custom manifest file/name convention was well received.

Potential use models are:

  1. --manifest=Manifest.1.8.toml: a strict name
  2. --manifest=path/to/Manifest.1.8.toml: a strict relative path
  3. --manifest=Manifest.#.#.toml: a magic name where the version is filled into the #'s
  4. --manifest=path/to/Manifest.#.#.toml: same magic but a relative path

There was also the idea of doing the same via a JULIA_MANIFEST_NAME env var, which might sidestep potential issues with #'s messing up the arg handling on some platforms?

cc. @GunnarFarneback @DilumAluthge @fredrikekre @StefanKarpinski

@DilumAluthge
Copy link
Member

I think we only need to worry about use cases 1 and 3, which is exactly what your PR #44286 does.

@IanButterworth IanButterworth deleted the ib/manifest_named_versions branch March 4, 2022 04:39
@IanButterworth IanButterworth restored the ib/manifest_named_versions branch November 30, 2023 18:18
@IanButterworth IanButterworth force-pushed the ib/manifest_named_versions branch from 6c3b200 to 87f6da8 Compare November 30, 2023 18:19
@IanButterworth IanButterworth marked this pull request as ready for review November 30, 2023 18:19
@IanButterworth
Copy link
Member Author

This has come up a few times recently, and seems to be the preferred approach over #44286

@IanButterworth IanButterworth changed the title RFC: Add preference for version named manifest files Add preference for version named manifest files Nov 30, 2023
@IanButterworth IanButterworth force-pushed the ib/manifest_named_versions branch from 87f6da8 to 7effd1b Compare December 14, 2023 02:43
@IanButterworth IanButterworth removed needs tests Unit tests are required for this change needs docs Documentation for this change is required needs news A NEWS entry is required for this change labels Dec 14, 2023
@IanButterworth
Copy link
Member Author

This is RTM from my perspective. I will merge this and bump Pkg immediately after merge.

Copy link
Member

@StefanKarpinski StefanKarpinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@IanButterworth IanButterworth merged commit e9b0fa1 into JuliaLang:master Dec 14, 2023
6 of 8 checks passed
@IanButterworth IanButterworth deleted the ib/manifest_named_versions branch December 14, 2023 19:55
@ufechner7
Copy link

Shall this be back-ported? If yes, to which Julia versions?

@LilithHafner LilithHafner removed the forget me not PRs that one wants to make sure aren't forgotten label Dec 15, 2023
@LilithHafner
Copy link
Member

Features are not usually backported.

If we don't backport this, and you are using both 1.9 and 1.10, then you'll have to keep dealing with the problems for now.

If you are using 1.10 and 1.11, though, then you can have a Manifest.toml file that is for 1.10 and a Manifest-v1.11.toml file that is for 1.11, and each Julia version will know to use the correct manifest file.

@vchuravy
Copy link
Member

vchuravy commented Feb 14, 2024

Is there a way to cause Julia to automatically create versioned Manifests? Would make switching versions a lot nicer.

Also an empty Manifest file is treated as v1

touch Manifest-v1.11.toml
┌ Warning: The active manifest file at `/home/vchuravy/src/GPUCompiler/Manifest-v1.11.toml` has an old format that is being maintained.
│ To update to the new format, which is supported by Julia versions ≥ 1.6.2, run `import Pkg; Pkg.upgrade_manifest()` which will upgrade the format without re-resolving.
│ To then record the julia version re-resolve with `Pkg.resolve()` and if there are resolve conflicts consider `Pkg.update()`.
└ @ Pkg.Types ~/.julia/juliaup/julia-nightly/share/julia/stdlib/v1.11/Pkg/src/manifest.jl:318

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.10 Change should be backported to the 1.10 release feature Indicates new feature / enhancement requests packages Package management and loading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants