Handle packages with local [sources] in test/Project.toml#37
Conversation
In Julia 1.13+, test dependencies often use [sources.PackageName] with path=".." to reference the main package. When Resolver.jl tries to resolve these packages from the registry, it fails with "unknown package UUID" for unregistered packages, or resolves to an old version for registered packages. This fix: - Detects packages with local path sources via [sources] section - Temporarily removes them from [deps], [compat], [extras], and [sources] before running Resolver.jl - Restores the original Project.toml after resolution - Excludes local source packages from forcedeps verification Fixes julia-actions#36 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
However, this does not resolve the issue in trixi-framework/Trixi.jl#2674 (comment), right? If I understand correctly, it does not merge package dependencies with test dependencies and resolves them together. That's fine if this PR resolves #36, but we should also fix merging package and test dependencies in another PR. Otherwise this action is not really useful I would say. |
When both "." and "test" are specified, merge their dependencies into a single project before running Resolver.jl. This ensures that the resolved versions are compatible when tests run (which combines both environments). The v1 action worked this way because it modified both Project.toml files before running tests. The v2 action was resolving them independently, which could result in incompatible versions when combined. Changes: - Add create_merged_project() to combine main and test deps - Add should_merge_projects() to detect when merging is needed - Merge deps, compat, and weakdeps from test into main project - Exclude local source packages (main package referenced via path) - Copy resulting manifest to main project directory - Fix scoping warnings in loops Addresses feedback from @JoshuaLampert in PR julia-actions#37. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@JoshuaLampert Thanks for the feedback! I've updated the PR to implement merged resolution. Now when both "." and "test" are specified as projects, the action will:
This matches how Julia's Example output: Note: There's still a separate Resolver.jl issue with extension handling for Julia 1.9+ ( |
|
Ok, interesting. This would need to be tested in the wild I guess. I'll see if I can run some tests on my packages using this version in the coming days. |
On its own it doesn't, because the manifest seems to be missing the package itself, see e.g. https://github.com/sethaxen/DowngradeTestPackage.jl/actions/runs/20273346969/job/58214984092#step:6:48 . However, on v1.12, if I add a Note that on v1.11, calling |
To be clear, "issue" here means "problem" correct? Because I could not find an issue on the Resolver.jl repo for this. |
I think AI got a bit confused. There is this issue: #25, which seems to be a problem of Resolver.jl. |
When the main package is excluded from resolution (because it has a local source in test/Project.toml), it was missing from the generated manifest. This caused issues because the test project depends on the main package but couldn't find it in the manifest. Now after copying the merged manifest to the main project directory, we append the main package as a path dependency with: - path = "." - uuid from Project.toml - version from Project.toml This fixes the issue reported by @sethaxen where the manifest was missing the package itself. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@sethaxen Thanks for testing! I've pushed a fix that adds the main package to the manifest after merged resolution. The issue was that when we exclude the main package from resolution (because it has a local source), it wasn't being added to the manifest. Now after copying the merged manifest, we append the main package as a path dependency with: [[deps.PackageName]]
path = "."
uuid = "..."
version = "..."Could you try the updated branch again?
Yes, I meant "problem" - sorry for the confusion. The MbedTLS_jll error is a bug in Resolver.jl's extension handling for Julia 1.9+ that's tracked in #25. |
Tests added: - "test/Project.toml with local sources": Verifies that packages with local [sources] entries are correctly detected and excluded from resolution, and the main package is added to the manifest as a path dependency. - "merged resolution with test dependencies": Verifies that when both main and test projects are specified, their dependencies are merged and minimized together, ensuring compatible versions when combined. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@JoshuaLampert I've added tests for the new functionality:
All 28 tests pass locally. |
sethaxen
left a comment
There was a problem hiding this comment.
I reviewed about half of the changes and have some suggestions and follow-up questions.
downgrade.jl
Outdated
| for (pkg_name, source_info) in sources | ||
| if source_info isa Dict && haskey(source_info, "path") | ||
| push!(local_pkgs, pkg_name) | ||
| @info "Found local source package: $pkg_name (path=$(source_info["path"]))" |
There was a problem hiding this comment.
Do we want this level of verbosity in the final PR?
There was a problem hiding this comment.
I don't think it's particularly bad to have more printed here
There was a problem hiding this comment.
I agree. I like the level of verbosity.
downgrade.jl
Outdated
| # Check for [sources] section entries with path keys | ||
| sources = get(project, "sources", Dict()) | ||
| for (pkg_name, source_info) in sources | ||
| if source_info isa Dict && haskey(source_info, "path") |
There was a problem hiding this comment.
Sources don't have to be specified by path. They can also be specified by url, see https://pkgdocs.julialang.org/v1/toml-files/#The-[sources]-section
downgrade.jl
Outdated
| mode in valid_modes || error("mode must be one of: $(join(valid_modes, ", "))") | ||
|
|
||
| """ | ||
| get_local_source_packages(project_file) |
There was a problem hiding this comment.
Should this be renamed? Source packages can also be remote (see below comment).
downgrade.jl
Outdated
| # Remove from [deps] | ||
| if haskey(project, "deps") | ||
| for pkg in local_pkgs | ||
| if haskey(project["deps"], pkg) | ||
| delete!(project["deps"], pkg) | ||
| modified = true | ||
| @info "Temporarily removing $pkg from [deps] for resolution" | ||
| end | ||
| end | ||
| end | ||
|
|
||
| # Remove from [extras] | ||
| if haskey(project, "extras") | ||
| for pkg in local_pkgs | ||
| if haskey(project["extras"], pkg) | ||
| delete!(project["extras"], pkg) | ||
| modified = true | ||
| @info "Temporarily removing $pkg from [extras] for resolution" | ||
| end | ||
| end | ||
| end | ||
|
|
||
| # Remove from [compat] | ||
| if haskey(project, "compat") | ||
| for pkg in local_pkgs | ||
| if haskey(project["compat"], pkg) | ||
| delete!(project["compat"], pkg) | ||
| modified = true | ||
| @info "Temporarily removing $pkg from [compat] for resolution" | ||
| end | ||
| end | ||
| end | ||
|
|
||
| # Remove from [sources] - must do this because Pkg validates that | ||
| # packages in [sources] must be in [deps] or [extras] | ||
| if haskey(project, "sources") | ||
| for pkg in local_pkgs | ||
| if haskey(project["sources"], pkg) | ||
| delete!(project["sources"], pkg) | ||
| modified = true | ||
| @info "Temporarily removing $pkg from [sources] for resolution" | ||
| end | ||
| end | ||
| # Remove empty [sources] section | ||
| if isempty(project["sources"]) | ||
| delete!(project, "sources") | ||
| end | ||
| end |
There was a problem hiding this comment.
This can be simplified by looping over all sections that need to be modified:
| # Remove from [deps] | |
| if haskey(project, "deps") | |
| for pkg in local_pkgs | |
| if haskey(project["deps"], pkg) | |
| delete!(project["deps"], pkg) | |
| modified = true | |
| @info "Temporarily removing $pkg from [deps] for resolution" | |
| end | |
| end | |
| end | |
| # Remove from [extras] | |
| if haskey(project, "extras") | |
| for pkg in local_pkgs | |
| if haskey(project["extras"], pkg) | |
| delete!(project["extras"], pkg) | |
| modified = true | |
| @info "Temporarily removing $pkg from [extras] for resolution" | |
| end | |
| end | |
| end | |
| # Remove from [compat] | |
| if haskey(project, "compat") | |
| for pkg in local_pkgs | |
| if haskey(project["compat"], pkg) | |
| delete!(project["compat"], pkg) | |
| modified = true | |
| @info "Temporarily removing $pkg from [compat] for resolution" | |
| end | |
| end | |
| end | |
| # Remove from [sources] - must do this because Pkg validates that | |
| # packages in [sources] must be in [deps] or [extras] | |
| if haskey(project, "sources") | |
| for pkg in local_pkgs | |
| if haskey(project["sources"], pkg) | |
| delete!(project["sources"], pkg) | |
| modified = true | |
| @info "Temporarily removing $pkg from [sources] for resolution" | |
| end | |
| end | |
| # Remove empty [sources] section | |
| if isempty(project["sources"]) | |
| delete!(project, "sources") | |
| end | |
| end | |
| # Remove from [deps], [extras], [compat], and [sources] | |
| for section_name in ("deps", "extras", "compat", "sources") | |
| haskey(project, section_name) || continue | |
| section = project["section_name"] | |
| for pkg in local_pkgs | |
| haskey(section, pkg) || continue | |
| delete!(section, pkg) | |
| modified = true | |
| @info "Temporarily removing $pkg from [$(section_name)] for resolution" | |
| end | |
| end | |
| # Remove empty [sources] section | |
| if haskey(project, "sources") && isempty(project["sources"]) | |
| delete!(project, "sources") | |
| end |
| end | ||
|
|
||
| if modified | ||
| open(project_file, "w") do io |
There was a problem hiding this comment.
I'm wondering if it's possible/better to write to a temporary project file instead of the actual project file. Will the resolver look at anything besides the project file while creating the manifest?
| merged = deepcopy(main_project) | ||
|
|
||
| # Remove workspace section (not needed for resolution) | ||
| delete!(merged, "workspace") |
There was a problem hiding this comment.
I think it's better to say that workspaces are not currently supported, and IMO a warning should be printed somewhere if a workspace is detected. Because in reality workspaces are recursively resolved, so a user could have e.g. a sub test environment test/integration/Project.toml and a workspace integration defined in test/Project.toml, and the main environment when resolved should take into account both of these test environments. Similarly, the user could have defined a dev workspace in Project.toml that takes into account both docs and tests. Since this current solution doesn't support any of these use cases, it should just print a warning to that effect.
| end | ||
|
|
||
| """ | ||
| create_merged_project(main_project_file, test_project_file, merged_dir) |
There was a problem hiding this comment.
Should this really be test project-specific? Presumably a user might want to do this for testing the docs build with downgraded packages as well. Or a separate integration test suite not at test/runtests.jl.
|
|
||
| # Process any remaining directories that aren't main or test | ||
| other_dirs = filter(d -> d != "." && d != "test", dirs) | ||
| for dir in other_dirs |
There was a problem hiding this comment.
This whole for loop is the same as below in the else branch. This can be modularized to avoid code duplication.
Co-authored-by: Seth Axen <seth@sethaxen.com>
Co-authored-by: Seth Axen <seth@sethaxen.com>
…rize code This commit addresses the review comments on PR julia-actions#37: 1. Renamed `get_local_source_packages` to `get_source_packages` since sources can be remote (URLs) not just local paths 2. Updated function to handle both `path` and `url` sources per https://pkgdocs.julialang.org/v1/toml-files/#The-[sources]-section 3. Renamed `remove_local_packages_from_project` to `remove_source_packages_from_project` for consistency 4. Simplified the section removal loop using a for loop over section names instead of duplicating code for deps, extras, compat, and sources 5. Added `resolve_directory` helper function to eliminate code duplication between the `other_dirs` loop and the `else` branch 6. Added `check_for_workspace` function to warn when non-standard workspaces are detected (nested environments, docs, etc. are not fully supported) 7. Applied JuliaFormatter with SciMLStyle All 28 tests pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
I've addressed the review feedback in commit 10eed4e: Changes made:
All 28 tests pass locally. Note: The verbosity level is kept as-is since @ChrisRackauckas and @JoshuaLampert both indicated they like the current level of logging. |
|
Sorry for the delay. I tried the new version again. I'm currently trying to make it work with the following Project.toml_ name = "DispersiveShallowWater"
uuid = "4ab875f6-b79a-4e28-9745-4f0293954817"
authors = ["Joshua Lampert <joshua.lampert@uni-hamburg.de>"]
version = "0.9.1-DEV"
[deps]
BandedMatrices = "aae01518-5342-5314-be14-df237901396f"
DelimitedFiles = "8bb1440f-4735-579b-a4ab-409b98df4dab"
DiffEqBase = "2b5f629d-d688-5b77-993f-72d75c75574e"
FastBroadcast = "7034ab61-46d4-4ed7-9d0f-46aef9175898"
ForwardDiff = "f6369f11-7733-5829-9624-2563aa707210"
Interpolations = "a98d9a8b-a2ab-59e6-89dd-64a1c18fca59"
LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e"
PolynomialBases = "c74db56a-226d-5e98-8bb0-a6049094aeea"
PreallocationTools = "d236fae5-4411-538c-8e31-a6e3d9e00b46"
Printf = "de0858da-6303-5e67-8744-51eddeeeb8d7"
RecipesBase = "3cdcf5f2-1ef4-517c-9805-6587b60abb01"
RecursiveArrayTools = "731186ca-8d62-57ce-b412-fbd966d074cd"
Reexport = "189a3867-3050-52da-a836-e630ba90ab69"
Roots = "f2b01f46-fcfa-551c-844a-d8ac1e96c665"
SciMLBase = "0bca4576-84f4-4d90-8ffe-ffa030f20462"
SimpleUnPack = "ce78b400-467f-4804-87d8-8f486da07d0a"
SparseArrays = "2f01184e-e22b-5df5-ae63-d93ebab69eaf"
StaticArrays = "90137ffa-7385-5640-81b9-e52037218182"
SummationByPartsOperators = "9f78cca6-572e-554e-b819-917d2f1cf240"
TimerOutputs = "a759f4b9-e2f1-59dc-863e-4aeb61b1ea8f"
TrixiBase = "9a0f1c46-06d5-4909-a5a3-ce25d3fa3284"
Aqua = "4c88cf16-eb10-579e-8560-4a9242c79595"
ExplicitImports = "7d51a73a-1435-4ff3-83d9-f097790105c7"
OrdinaryDiffEqLowStorageRK = "b0944070-b475-4768-8dec-fb6eb410534d"
OrdinaryDiffEqRosenbrock = "43230ef6-c299-4910-a778-202eb28ce4ce"
OrdinaryDiffEqTsit5 = "b1df2697-797e-41e3-8120-5422d3b24e4a"
Plots = "91a5bcdd-55d7-5caf-9e0b-520d859cae80"
Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
TestItemRunner = "f8b46487-2199-4994-9208-9a1283c18c0a"
TestItems = "1c621080-faea-4a02-84b6-bbd5e436b8fe"
TrixiTest = "0a316866-cbd0-4425-8bcb-08103b2c1f26"
[compat]
BandedMatrices = "1.5"
DelimitedFiles = "1"
DiffEqBase = "6.160"
FastBroadcast = "0.3.5"
ForwardDiff = "0.10.36, 1"
Interpolations = "0.15.1, 0.16"
LinearAlgebra = "1"
PolynomialBases = "0.4.15"
PreallocationTools = "0.4.23, 1"
Printf = "1"
RecipesBase = "1.3.4"
RecursiveArrayTools = "3.27.2"
Reexport = "1.2.2"
Roots = "2.0.17"
SciMLBase = "2.78"
SimpleUnPack = "1.1"
SparseArrays = "1"
StaticArrays = "1.9.7"
SummationByPartsOperators = "0.5.79"
TimerOutputs = "0.5.25"
TrixiBase = "0.1.6"
Aqua = "0.7, 0.8"
ExplicitImports = "1.0.1"
OrdinaryDiffEqLowStorageRK = "1.1"
OrdinaryDiffEqRosenbrock = "1.9"
OrdinaryDiffEqTsit5 = "1.1"
Plots = "1.25"
Test = "1"
TestItemRunner = "1"
TestItems = "1"
TrixiTest = "0.1.3, 0.2"
julia = "1.10"(I manually merged the Project.toml and test/Project.toml for simplicity.)
This looks like #25 and would be nice to get fixed.
The UUID 8bb1440f-4735-579b-a4ab-409b98df4dab is the one of DelimitedFiles.jl, which is in the list of ignored packages. So I would not expect it to make any problems. Finally, using julia v1.12 seems to work (it fails due to incompatibilities with Plots.jl, but I tried the same with v1 of this action and julia v1.12 and it was also failing). So I guess that is fine. I would still like to make this work with julia v1.10 in order to test the exact same setup as with v1 of the action. |
|
I also have been meaning to check that this works as well if the docs environment also includes the main package as a source. |
|
I created ChrisRackauckas-Claude#1 to fix #25 and the problems I got above. Now looks almost correct and almost recovers what v1 did with one exception: Resolver.jl resolves to Plots.jl v1.38.9 instead of v1.25.12, which v1 did and follows the minimum allowed version (considering the [deps]
Plots = "91a5bcdd-55d7-5caf-9e0b-520d859cae80"
[compat]
Plots = "1.25.12"
julia = "1.10"We see: $ julia +1.10 --project=@temp --startup-file=no --color=yes downgrade.jl "" "." "forcedeps" "1.10"
[ Info: Using Resolver.jl with mode: forcedeps
┌ Warning: The project dependencies or compat requirements have changed since the manifest was last resolved.
│ It is recommended to `Pkg.resolve()` or consider `Pkg.update()` if necessary.
└ @ Pkg.API ~/.julia/juliaup/julia-1.10.10+0.x64.linux.gnu/share/julia/stdlib/v1.10/Pkg/src/API.jl:1142
[ Info: Running resolver on . with --min=@deps
[ Info: Successfully resolved minimal versions for .
[ Info: Checking that resolved versions match forced lower bounds for ....
┌ Error: forcedeps check failed: Plots resolved to 1.38.9 but lower bound is 1.25.12
└ @ Main ~/testproject/downgrade.jl:489
ERROR: LoadError: forcedeps check failed for .: Some packages did not resolve to their lower bounds.
This means the lowest compatible versions of your direct dependencies are
incompatible with each other. To fix this, you need to increase the lower
bounds in your compat entries to versions that are mutually compatible.
See the errors above for which packages need their bounds adjusted.
Stacktrace:
[1] error(s::String)
@ Base ./error.jl:35
[2] resolve_directory(dir::SubString{String}, resolver_path::String, resolver_mode::String, julia_version::String, mode::String, ignore_pkgs::Vector{SubString{String}})
@ Main ~/testproject/downgrade.jl:305
[3] top-level scope
@ ~/testproject/downgrade.jl:590
in expression starting at ~/testproject/downgrade.jl:512However, using Pkg.jl's resolver shows it is possible to pick Plots.jl v1.25.12 by enforcing it with $ julia +1.10 --project=.
_
_ _ _(_)_ | Documentation: https://docs.julialang.org
(_) | (_) (_) |
_ _ _| |_ __ _ | Type "?" for help, "]?" for Pkg help.
| | | | | | |/ _` | |
| | |_| | | | (_| | | Version 1.10.10 (2025-06-27)
_/ |\__'_|_|_|\__'_| | Official https://julialang.org/ release
|__/ |
(testproject) pkg> resolve
[...]
(testproject) pkg> st
Status `~/testproject/Project.toml`
⌅ [91a5bcdd] Plots v1.25.12
Info Packages marked with ⌅ have new versions available but compatibility constraints restrict them from upgrading. To see why use `status --outdatedThis means Resolver.jl picks a newer version of Plots.jl even though it could pick an older one and therefore the forcedeps check fails. After some more digging, I think this is because Resolver.jl by default tries to maximize all the indirect dependencies. Thus, it wants to pick a new version of GR.jl (v0.72.10), which in turn is not compatible with Plots.jl <v1.38.9, see JuliaPlots/Plots.jl@ |
|
And another note: The current implementation of the forcedeps mode corresponds to the |
Add `julia_version = 1` conversion and run resolver with `Base.julia_cmd`
Summary
[sources]section and temporarily excludes them from resolutionProblem
In Julia 1.13+, test dependencies use
[sources.PackageName]withpath=".."to reference the main package. When Resolver.jl tries to resolve these packages from the registry:Reported in #36 and trixi-framework/Trixi.jl#2674 (comment)
Solution
[sources]section[deps],[compat],[extras], and[sources]Test plan
[sources.PackageName]withpath=".."Note
There's a separate issue in Resolver.jl when handling workspace projects with Julia 1.9+ extension handling (MbedTLS_jll error in
fixups_from_projectfile!). This is unrelated to this fix and should be addressed in Resolver.jl.🤖 Generated with Claude Code