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.

Sign up for GitHub

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

Jump to bottom

New manifest format: Fix upgrade_manifest and raw dict entry for nothing #2610

Merged
merged 8 commits into from
Jun 9, 2021
Merged

New manifest format: Fix upgrade_manifest and raw dict entry for nothing #2610

merged 8 commits into from
Jun 9, 2021

Conversation

DilumAluthge
Copy link
Member

Fixes #2609

@IanButterworth
Copy link
Member

Shoot. I thought this was covered by tests. I'll push a test

@DilumAluthge
Copy link
Member Author

@vtjnash If you get a chance, could you try this PR branch with your manifest from #2609 and see if it works? It should, but it'd be good to double-check.

@DilumAluthge
Copy link
Member Author

Also @IanButterworth does this need backporting to both 1.6 and 1.7, or just 1.7?

@IanButterworth
Copy link
Member

Just 1.7

@DilumAluthge
Copy link
Member Author

Shoot. I thought this was covered by tests. I'll push a test

You can push directly to this branch, if that works for you.

KristofferC
KristofferC reviewed Jun 9, 2021
View reviewed changes
src/manifest.jl Outdated Show resolved Hide resolved
@DilumAluthge
Copy link
Member Author

CI is passing now.

@DilumAluthge DilumAluthge requested review from IanButterworth and KristofferC and removed request for IanButterworth June 9, 2021 20:13
@IanButterworth
Copy link
Member

Wait. There's a bug

@DilumAluthge
Copy link
Member Author

Good to merge now?

@DilumAluthge DilumAluthge changed the title New manifest format: Handle the case where julia_version is the string "nothing" in New manifest format: Handle the case where julia_version is the string "nothing" Jun 9, 2021
@IanButterworth IanButterworth changed the title New manifest format: Handle the case where julia_version is the string "nothing" New manifest format: Fix upgrade_manifest and raw dict entry for nothing Jun 9, 2021
@IanButterworth IanButterworth merged commit b8bea6c into master Jun 9, 2021
@IanButterworth IanButterworth deleted the dpa/fix_upgrade_manifest branch June 9, 2021 21:52
KristofferC pushed a commit that referenced this pull request Jun 10, 2021
@IanButterworth @KristofferC
New Manifest.toml format: Pt 1. Teach Pkg new format, but default to …
c793378 
…old format (#2561)

* implement manifest format with deps field

* temporarily run CI on base PR

* simpler convert_flat_format_manifest

* default to old manifest format

* remove warning about maintaining old manifest format

* add unknown manifest format warning

* fix

* only print the major and minor part of the `manifest_format` to file

* add tests for manifest.toml formats

* add test for unknown manifest format warning

* provide manifest file path in warning, when available

* Revert "temporarily run CI on base PR"

This reverts commit d4a1e14.

* add support for unknown extra data in Manifest

* fixes

* add missing write_manifest method

* fix with windows-safe regex

Co-authored-by: KristofferC <[email protected]>
(cherry picked from commit a3fc759)

New manifest format: Fix `upgrade_manifest` and raw dict entry for nothing (#2610)

(cherry picked from commit b8bea6c)
@StefanKarpinski
Copy link
Member

Alternatively, could we just leave this key out when we don't know the Julia version? Using "nothing" like Julia's nothing feels like it is a bit of a violation of the TOML abstraction: TOML data should be meaningful and interpretable without knowing about Julia's type system; using the string "nothing" like this leaks the Julia data model into TOML land in a kind of unfortunate way. It seems like it would be cleaner for the julia_version entry to be absent instead.

@IanButterworth
Copy link
Member

That sounds good to me. It wouldn't break the base heuristic
https://github.com/JuliaLang/julia/blob/15c19c8a6b1c1a87e6dd24fd058de5192826b250/base/loading.jl#L560-L570

But we would want to get that change out before people start generating manifests with julia_version = "nothing" in it, so could block the 1.7 beta?

@IanButterworth IanButterworth mentioned this pull request Jun 10, 2021
Closed
@StefanKarpinski
Copy link
Member

I wouldn't worry about the people who are using master, just tell them how to fix it when it breaks.

KristofferC pushed a commit that referenced this pull request Jun 16, 2021
@DilumAluthge @KristofferC
New manifest format: Fix upgrade_manifest and raw dict entry for no…
e846f61 
…thing (#2610)

(cherry picked from commit b8bea6c)
KristofferC pushed a commit that referenced this pull request Jun 16, 2021
@DilumAluthge @KristofferC
New manifest format: Fix upgrade_manifest and raw dict entry for no…
27ab070 
…thing (#2610)

(cherry picked from commit b8bea6c)
@IanButterworth IanButterworth mentioned this pull request Jun 26, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash left review comments

@IanButterworth IanButterworth Awaiting requested review from IanButterworth

@KristofferC KristofferC Awaiting requested review from KristofferC

Assignees
No one assigned
Labels
bugfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pkg.upgrade_manifest broke my Manifest
5 participants
@DilumAluthge @IanButterworth @StefanKarpinski @vtjnash @KristofferC