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

Backports for 1.6, take 2. #2628

Merged
merged 4 commits into from
Jun 29, 2021
Merged

Backports for 1.6, take 2. #2628

merged 4 commits into from
Jun 29, 2021

Conversation

KristofferC
Copy link
Member

No description provided.

@KristofferC KristofferC force-pushed the kc/1.6 branch 3 times, most recently from 36fdfa1 to 7228f38 Compare June 16, 2021 21:28
@IanButterworth
Copy link
Member

I tested this locally against JuliaLang/julia#40702 and got the same error

undo redo functionality: Test Failed at /Users/ian/Documents/GitHub/Pkg.jl/test/pkg.jl:700
  Expression: !(haskey(Pkg.dependencies(), unicode_uuid))

No clue..

@IanButterworth
Copy link
Member

@KristofferC I've looked at this a few times and still don't have any idea what broke the undo tests

@IanButterworth
Copy link
Member

IanButterworth commented Jun 26, 2021

2018f15 is the first bad commit for the undo tests, testing with JuliaLang/julia#40702

Also, 27ab070 seems like a bad backport as Pkg.upgrade_manifest() wasn't backported to 1.6 but that backport add a test for it, so that'll fail.
I don't think what was in that original PR is needed at all as there are later commits that fix the other changes it made. So I'd just drop it.

@IanButterworth
Copy link
Member

IanButterworth commented Jun 26, 2021

I think the problem is that the manifest equality is failing here https://github.com/JuliaLang/Pkg.jl/blob/kc/1.6/src/API.jl#L1539

I guess during the undo tests the original_manifest that's deepcopied during the intial Context() is missing the julia_version which is added in the initial resolve during the first Pkg.add, so every comparison afterwards back to that will result in ctx.manifest == ctx.original_manifest being false, which means the undo snapshot will always be added.

Perhaps == for manifests should just check the deps?
PR: #2649

Though, I don't yet understand why the undo tests aren't failing on master..

Edit: On second thought.. I'm really not certain I've got a handle on this properly. When original_manifest is updated throughout the EnvCaching is confusing me

Edit edit: Seems like my suggestion passes #2650 but still not clear why it didn't fail on master

@IanButterworth
Copy link
Member

IanButterworth commented Jun 26, 2021

The fix agreed with @KristofferC was to change the undo functionality to just consider equality wrt. the manifest deps, rather than change the equality method for the manifest type to do that

@DilumAluthge
Copy link
Member

DilumAluthge commented Jun 29, 2021

@KristofferC @fredrikekre Is this good to go?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release Release management and versioning.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants