New Manifest.toml format: Pt 1. Teach Pkg new format, but default to old format #2561

IanButterworth · 2021-05-12T02:54:19Z

Initial discussion in #2557

This is Part 1, which is intended to be backported to 1.6.2

Teach Pkg new manifest format with julia_version and manifest_format metadata fields, and package deps nested under a deps field
Still defaults to old format
Maintains format of any existing Manifest.toml
Warn if unknown manifest_format version is detected

Part 2: #2580

Dependent on Add support for new Manifest.toml format during code load julia#40765 - ~~GitHub actions CI has been limited to linux 64 and that PR's binary is used (ignore appveyor)~~
Needs explicit tests for old/new/unknown manifest formats
Needs tests for invalid manifest formats

src/manifest.jl

src/Types.jl

IanButterworth · 2021-05-30T06:29:26Z

Updates:

Added julia_version and manifest_format fields
The original idea was to keep this to just moving the packages to under a deps field, but I think it makes more sense from the Pkg call to properly version the format, and thus including manifest_format as a VersionNumber string seems the right way to do it. And introducing this new format as manifest format 2 it didn't seem to make sense to omit the julia_version field, given that's the main motivator for making this switch
Now the format of the manifest file is maintained when re-written.
A @warn hints once (via maxlog=1) how the user can update to the new manifest format.

Beyond reviewing, I think it would be good for others to try this PR out, on top of JuliaLang/julia#40765

Btw this is marked as do not merge to remind me that CI has been messed with.

src/manifest.jl

KristofferC · 2021-05-30T10:36:29Z

Some thoughts:

I am not sure the warning should be added, at least not right away.
Perhaps there should be a good error when the manifest version is >= 3?
It would be good to add a test that shows that it is now possible to read the new format as well as that writing it works.

IanButterworth · 2021-05-30T13:27:47Z

I am not sure the warning should be added, at least not right away.

I feel like a warning should be in 1.7 but maybe how this PR is right now isn't exactly what's needed for the backport to 1.6.2

For the backport I think it needs to default to writing to v1 format, and doesn't need to warn if it's maintaining a v2 manifest.

I can make the goal of this PR be the backport which can then be built on.

Perhaps there should be a good error when the manifest version is >= 3?

Perhaps a warning? "Unknown newer Manifest.toml format version detected, unexpected behavior may occur"

It would be good to add a test that shows that it is now possible to read the new format as well as that writing it works.

Yeah agreed. Also need to add tests to the Base PR

IanButterworth · 2021-05-30T19:57:17Z

Updated based on feedback.

In #2557 @davidanthoff suggested schemaversion for what is currently manifest_format. Would that be a better name?

Currently

# This file is machine-generated - editing it directly is not advised

julia_version = "1.7.0-DEV.1199"
manifest_format = "2.0.0"

[[deps.ArgTools]]
uuid = "0dad84c5-d112-42e6-8d28-ef12dabb789f"
...

davidanthoff · 2021-05-30T20:04:04Z

Just a minor question: does it make sense to use three-part versions for the manifest/schema version (whatever the name is, I don't really have a preference)? I think semver is not that useful for format versioning, or at least it is not entirely clear to me how one would use it for that. Maybe better to just use an integer for that and keep it simple?

IanButterworth · 2021-05-30T20:08:03Z

Yeah three part is definitely excessive, but two might be useful. It only prints 3-part because that's the way VersionNumbers print in the TOML writer.

I'll convert to a 2-part string during the destructure before write. Also, happy to just do a single integer if others agree

Edit: Tests pass. The two "Run tests" ones are the only valid tests for now

KristofferC · 2021-06-01T06:09:58Z

src/Types.jl

+
+Base.@kwdef mutable struct Manifest
+    julia_version::Union{Nothing,VersionNumber} = Base.VERSION
+    manifest_format::VersionNumber = v"1.0.0" # default to older flat format


Is the Manifest constructor actually ever called without these arguments being explicitly given? If not, I suggest the defaults are removed.

It is called, when a manifest file doesn't exist https://github.com/IanButterworth/Pkg.jl/blob/8281b20d232768e0bcc4f28eb85965ffdd8a94e1/src/manifest.jl#L176

src/manifest.jl

This reverts commit d4a1e14.

IanButterworth · 2021-06-02T13:17:22Z

CI needs re-running when the merged JuliaLang/julia#40765 works its way into the nightlies

DilumAluthge · 2021-06-03T04:33:00Z

@KristofferC @StefanKarpinski Is this PR good to merge once CI turns green?

KristofferC · 2021-06-03T07:34:01Z

LGTM now

IanButterworth · 2021-06-06T17:39:39Z

Windows has caught up, but there seems to be an unrelated issue popping up on MacOS sometimes #2596

DilumAluthge · 2021-06-07T02:11:06Z

@fredrikekre or @IanButterworth Can you add this PR to the backports PR (#2592)?

…old format (JuliaLang#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]>

IanButterworth · 2021-06-07T03:21:01Z

#2575 will need backporting before this. And might as well do #2581 too.
I just gave it a go and it needs a little help. I'll give it another go unless @fredrikekre gets there first

KristofferC · 2021-06-07T07:51:40Z

It would be good to clean up the commit message a bit when squash merging something big like this.

…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)

IanButterworth requested a review from KristofferC May 12, 2021 02:54

IanButterworth requested a review from a team as a code owner May 12, 2021 02:54

IanButterworth mentioned this pull request May 12, 2021

WIP: Implement new manifest format with top-level metadata #2557

Closed

DilumAluthge removed the request for review from a team May 12, 2021 03:00

IanButterworth force-pushed the ib/new_manifest_minimum branch 2 times, most recently from 322e82e to dcebfee Compare May 12, 2021 04:16

fredrikekre reviewed May 12, 2021

View reviewed changes

src/manifest.jl Show resolved Hide resolved

IanButterworth added the DO NOT MERGE Do not merge this PR! label May 13, 2021

IanButterworth commented May 18, 2021

View reviewed changes

src/Types.jl Show resolved Hide resolved

IanButterworth force-pushed the ib/new_manifest_minimum branch 2 times, most recently from 1fa2217 to 9bc451e Compare May 30, 2021 06:05

implement manifest format with deps field

4a03800

IanButterworth force-pushed the ib/new_manifest_minimum branch from 9bc451e to 56cc7c2 Compare May 30, 2021 06:23

KristofferC reviewed May 30, 2021

View reviewed changes

src/manifest.jl Show resolved Hide resolved

IanButterworth mentioned this pull request May 30, 2021

Add support for new Manifest.toml format during code load JuliaLang/julia#40765

Merged

1 task

temporarily run CI on base PR

d4a1e14

IanButterworth force-pushed the ib/new_manifest_minimum branch from 56cc7c2 to d4a1e14 Compare May 30, 2021 19:20

IanButterworth changed the title ~~Core implementation of manifest format with package entries under deps field~~ New Manifest.toml format: Pt 1. Teach Pkg new format, but default to old format May 30, 2021

IanButterworth added 5 commits May 30, 2021 15:37

simpler convert_flat_format_manifest

57306cd

default to old manifest format

56719db

remove warning about maintaining old manifest format

cfbaa97

add unknown manifest format warning

4d6e044

fix

e1b227b

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

261ec20

add test for unknown manifest format warning

8281b20

KristofferC reviewed Jun 1, 2021

View reviewed changes

src/manifest.jl Outdated Show resolved Hide resolved

fonsp mentioned this pull request Jun 1, 2021

🎁 Built-in Pkg management fonsp/Pluto.jl#844

Merged

66 tasks

IanButterworth added 2 commits June 2, 2021 09:11

provide manifest file path in warning, when available

32fe1f5

Revert "temporarily run CI on base PR"

c9eb847

This reverts commit d4a1e14.

DilumAluthge closed this Jun 3, 2021

DilumAluthge reopened this Jun 3, 2021

DilumAluthge requested review from KristofferC and StefanKarpinski June 3, 2021 04:32

IanButterworth mentioned this pull request Jun 3, 2021

Handle new Manifest.toml format timholy/Revise.jl#629

Merged

add support for unknown extra data in Manifest

2860383

IanButterworth added 2 commits June 3, 2021 19:50

fixes

e9ab825

add missing write_manifest method

2058892

IanButterworth removed the DO NOT MERGE Do not merge this PR! label Jun 4, 2021

fix with windows-safe regex

26dad55

IanButterworth mentioned this pull request Jun 6, 2021

Nested Pkg.test fails on MacOS CI with git registry - GitError(Code:ELOCKED, Class:OS, failed remove #2596

Closed

DilumAluthge merged commit a3fc759 into JuliaLang:master Jun 7, 2021

IanButterworth deleted the ib/new_manifest_minimum branch June 7, 2021 03:21

fredrikekre removed the backport pending 1.6 label Jun 11, 2021

ericphanson mentioned this pull request Aug 20, 2021

Start specific Julia version if it is specified in Project.toml/Manifest.toml JuliaLang/juliaup#10

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Manifest.toml format: Pt 1. Teach Pkg new format, but default to old format #2561

New Manifest.toml format: Pt 1. Teach Pkg new format, but default to old format #2561

IanButterworth commented May 12, 2021 •

edited

Loading

IanButterworth commented May 30, 2021 •

edited

Loading

KristofferC commented May 30, 2021 •

edited

Loading

IanButterworth commented May 30, 2021 •

edited

Loading

IanButterworth commented May 30, 2021

davidanthoff commented May 30, 2021

IanButterworth commented May 30, 2021 •

edited

Loading

KristofferC Jun 1, 2021

IanButterworth Jun 2, 2021

IanButterworth commented Jun 2, 2021

DilumAluthge commented Jun 3, 2021

KristofferC commented Jun 3, 2021

IanButterworth commented Jun 6, 2021

DilumAluthge commented Jun 7, 2021

IanButterworth commented Jun 7, 2021 •

edited

Loading

KristofferC commented Jun 7, 2021

New Manifest.toml format: Pt 1. Teach Pkg new format, but default to old format #2561

New Manifest.toml format: Pt 1. Teach Pkg new format, but default to old format #2561

Conversation

IanButterworth commented May 12, 2021 • edited Loading

IanButterworth commented May 30, 2021 • edited Loading

KristofferC commented May 30, 2021 • edited Loading

IanButterworth commented May 30, 2021 • edited Loading

IanButterworth commented May 30, 2021

davidanthoff commented May 30, 2021

IanButterworth commented May 30, 2021 • edited Loading

KristofferC Jun 1, 2021

Choose a reason for hiding this comment

IanButterworth Jun 2, 2021

Choose a reason for hiding this comment

IanButterworth commented Jun 2, 2021

DilumAluthge commented Jun 3, 2021

KristofferC commented Jun 3, 2021

IanButterworth commented Jun 6, 2021

DilumAluthge commented Jun 7, 2021

IanButterworth commented Jun 7, 2021 • edited Loading

KristofferC commented Jun 7, 2021

IanButterworth commented May 12, 2021 •

edited

Loading

IanButterworth commented May 30, 2021 •

edited

Loading

KristofferC commented May 30, 2021 •

edited

Loading

IanButterworth commented May 30, 2021 •

edited

Loading

IanButterworth commented May 30, 2021 •

edited

Loading

IanButterworth commented Jun 7, 2021 •

edited

Loading