Skip to content

Backport fixes for conflicts between [workspace] and [sources] relative paths to release-1.12#4513

Merged
KristofferC merged 4 commits intoJuliaLang:release-1.12from
MasonProtter:mp/backport-sources-fix
Nov 17, 2025
Merged

Backport fixes for conflicts between [workspace] and [sources] relative paths to release-1.12#4513
KristofferC merged 4 commits intoJuliaLang:release-1.12from
MasonProtter:mp/backport-sources-fix

Conversation

@MasonProtter
Copy link
Copy Markdown
Contributor

@MasonProtter MasonProtter commented Nov 13, 2025

Backport of #4427 which fixes conflicts between [workspaces] and [sources] (e.g. #3842).

In order to to get it, I also needed to backport #4225 and #4229

I also needed to modify the CI to run on v1.12-nightly instead of nightly, since otherwise the REPL extension failed to precompile.

@github-project-automation github-project-automation Bot moved this to New in Pkg.jl Nov 13, 2025
@MasonProtter MasonProtter force-pushed the mp/backport-sources-fix branch 2 times, most recently from f7eb74c to 661a2ea Compare November 14, 2025 11:26
@MasonProtter MasonProtter requested a review from a team as a code owner November 14, 2025 11:26
@MasonProtter MasonProtter force-pushed the mp/backport-sources-fix branch from 661a2ea to 2233e1a Compare November 14, 2025 12:02
@MasonProtter MasonProtter changed the title Attempt to backport #4427 Backport #4225, #4229, #4427 to release-1.12 Nov 14, 2025
@MasonProtter MasonProtter changed the title Backport #4225, #4229, #4427 to release-1.12 Backport fixes for conflicts between [workspace] and [sources] relative paths to release-1.12 Nov 14, 2025
@KristofferC
Copy link
Copy Markdown
Member

fb8b74d should not be in, right? That's a feature.

@MasonProtter
Copy link
Copy Markdown
Contributor Author

MasonProtter commented Nov 14, 2025

So at least with my local testing, I couldn't get this to work without that commit, presumably because of the way it modified update_source_if_set(env, pkg)

But maybe I was just being dumb, or there's a more surgical way we can take only the parts of that commit that are needed to make this work. I'll try a branch without it again and see what happens.

@MasonProtter
Copy link
Copy Markdown
Contributor Author

Ugh looks like there actually is a problem here too that's not just on Windows, I had accidentally only tested the version that didn't have the commit I actually wanted 🤦. Sorry for the premature ping on Slack.

@MasonProtter MasonProtter force-pushed the mp/backport-sources-fix branch from a0846c2 to 1a2b5ac Compare November 14, 2025 14:03
@MasonProtter
Copy link
Copy Markdown
Contributor Author

Okay, so this is actually looking promising other than 2 things:

  1. the [sources] test in https://github.com/MasonProtter/Pkg.jl/blob/mp/backport-sources-fix/test/sources.jl#L24 was expecting
(Pkg.project()).sources["Example"] == Dict("url" => "https://github.com/JuliaLang/Example.jl")

but got

Dict(rev = "master", "url" => "https://github.com/JuliaLang/Example.jl")

which I think is okay if I'm understanding correctly, because that's what we are already doing on the master branch: https://github.com/JuliaLang/Pkg.jl/blob/master/test/sources.jl#L25, so I will update the test to do that

  1. The workspace tests are failing at https://github.com/JuliaLang/Pkg.jl/blob/1a2b5ac0b81605c0a56fcff264520073d2fc4c03/test/workspaces.jl#L142C17-L142C47 during generate because we already generate'd PrivatePackage earlier in the test suite, so I think that can just be removed?

@MasonProtter
Copy link
Copy Markdown
Contributor Author

MasonProtter commented Nov 14, 2025

Okay, sorry for the noise previously, @KristofferC. This was my first time doing manual backporting and I made some dumb mistakes along the way, but I think it's looking good now.

@MasonProtter MasonProtter force-pushed the mp/backport-sources-fix branch 3 times, most recently from 535f002 to c4856a8 Compare November 14, 2025 17:04
@MasonProtter
Copy link
Copy Markdown
Contributor Author

Okay, I think I correctly got rid of that stray change to pin. It looks like what happened was that I messed up my local git history, and that change from pin got moved onto the wrong commit, so when I cherry picked the commit, it brought that change along with it... New to git cherry-pick, sorry!

@MasonProtter
Copy link
Copy Markdown
Contributor Author

Hmm, that change to pin was actually necessary for some of the tests. Should I change the tests, or backport the change to pin?

@MasonProtter MasonProtter force-pushed the mp/backport-sources-fix branch from f975286 to fea1961 Compare November 14, 2025 18:31
@MasonProtter
Copy link
Copy Markdown
Contributor Author

Bump @KristofferC

@KristofferC
Copy link
Copy Markdown
Member

Is the diff in 55455a2 weird? It shows it adds a large number of workspace tests but those are already on 1.12 as far as I can see?

@KristofferC
Copy link
Copy Markdown
Member

Actually, I think the whole testing block in the workspace file has gotten duplicated?

We should run runic on the 1.12 branch as well so that backports are conflicting less.

@MasonProtter MasonProtter force-pushed the mp/backport-sources-fix branch from fea1961 to a99f4a6 Compare November 17, 2025 14:39
Copy link
Copy Markdown
Member

@KristofferC KristofferC left a comment

Choose a reason for hiding this comment

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

Looks good now I think

Copy link
Copy Markdown
Member

@KristofferC KristofferC left a comment

Choose a reason for hiding this comment

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

Looks good now I think

@github-project-automation github-project-automation Bot moved this from New to In review in Pkg.jl Nov 17, 2025
@MasonProtter
Copy link
Copy Markdown
Contributor Author

Okay, so I started over again with a fresh clone and re-did the cherry-picks.

  1. It turns out that the pin change was in the commit 2097cdb afterall, I don't know why Github was telling us before that it wasn't, so I've re-associated it with that cherry-picked commit
  2. Instead of letting git's diff stuff handle the conflicts with the workspace test (sorry about that, I should have checked it more carefully), I just rejected the automatic diff and manually applied the change in https://github.com/JuliaLang/Pkg.jl/pull/4427/files#diff-f95997eecce5f2754dd5a12ee49dc25041c2b377cb8239112ec267a5abf7f0c0, I think that should do the right thing

I think the reason the diff was messed up for the workspace was that it merged funny. i.e., compare the diff in the link above against the diff in the merge commit: 55455a2#diff-f95997eecce5f2754dd5a12ee49dc25041c2b377cb8239112ec267a5abf7f0c0


Regarding Runic, do you want it applied after these commits, or before them?

@KristofferC
Copy link
Copy Markdown
Member

KristofferC commented Nov 17, 2025

Regarding Runic, do you want it applied after these commits, or before them?

Neither, it is just something that we should do later, to simplify future backports.

@MasonProtter
Copy link
Copy Markdown
Contributor Author

Sounds good. Thanks for your patience as I bumbled through this! :P

@KristofferC KristofferC merged commit 98a52b1 into JuliaLang:release-1.12 Nov 17, 2025
5 of 7 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in Pkg.jl Nov 17, 2025
@KristofferC
Copy link
Copy Markdown
Member

JuliaLang/julia#60158

@KristofferC
Copy link
Copy Markdown
Member

I think this enabled auto adding of source entries to Project.toml when using e.g. dev. I was kind of worried about that to be honest but I repressed it... 😭

@MasonProtter
Copy link
Copy Markdown
Contributor Author

MasonProtter commented Nov 25, 2025

Ah crap, yeah it does appear to do that. I think this is the same problem I was having with pin and the workspace change, where the merge commit 2097cdb is different from the PR diff in #4229 so when I cherry picked that merge commit, I ended up taking chunks of code from that sources PR...

Uh, what's the best way to address this now that it's released?

@KristofferC
Copy link
Copy Markdown
Member

Revert this and 1.12.3 I guess...

@MasonProtter
Copy link
Copy Markdown
Contributor Author

I've opened a PR to try and address the problem without reverting the whole thing: #4513

KristofferC pushed a commit that referenced this pull request Nov 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants