-
Notifications
You must be signed in to change notification settings - Fork 39
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
Don't make PRs to packages that have a dependency that upper bounds the package CompatHelper bumps #160
Comments
I agree. I’m just not sure how to implement this. |
What about:
That should get you the latest possible versions of all packages while respecting the upper bounds of dependencies, right? |
It's like what @colinxs already said but, instead of adding this feature in CompatHelper.jl, can we have a mode in |
I'm happy with exploring different non-default modes for the resolver. The logic would be around here: |
It is worth figuring out how to reduce these notifications. Certainly one solution would be to have a single pull request for all suggested changes, instead of a different pull request for each suggested change: #126 |
That would be a great change. But waiting to submit the PR until the resolver will actually use the version whose bounds are loosening is also useful. The main reason these are hanging around in my inbox is that these are the ones I couldn't deal with immediately because I needed to work my way up the hierarchy of dependencies. |
I wanted to share a port of CompatHelper that I've been using internally which addresses this issue. Rather than deleting the existing compat entries like I described above, it replaces them with inequality specifiers. So a compat entry that looked like:
would get replaced with:
before calling Some other differences that I can think of:
I originally intended on split this up into PRs to CompatHelper, but it doesn't share much code since I'm relying on |
I don’t think that is sufficient. In your example, your bot makes a PR that replaces this line:
With this line:
There is no guarantee that the test suite will use Distributions 0.23. This is the same problem that CompatHelper has, so I’m not sure how this approach solves it. (Keep in mind that at the beginning of the test suite, when the test dependencies are resolved, the resolver is allowed to also change the versions of the direct dependencies.) The core issue here is that when you run the test suite, you need to enforce that Distributions 0.23 is used. |
Just to elaborate: even if |
I don’t mean to be negative! It’s a good idea, I just don’t think it solves the core problem. I really like the markdown output, the single branch for all changes, and the option to use the PAT. It would be great to have those features in CompatHelper, and I’d be happy to accept PRs for those features. |
You're not being negative :) Perhaps I'm misinterpreting, but from the
So after the call to Of course if you don't check in a Based on JuliaLang/Pkg.jl#1233 and other discussions, it sounds like the goal is to have |
So, there are two problems here. The first problem is that many people (myself included) do not check in the Therefore, we cannot have a solution that requires people to check The second problem is that those docs are out-of-date and are no longer correct. I have set up an example package to demonstrate: Look at the name = "Foo"
uuid = "8f191a96-946f-40fe-b50f-e281de84ebcd"
authors = ["Dilum Aluthge <[email protected]>"]
version = "0.1.0"
[deps]
StatsBase = "2913bbd2-ae8a-5f71-8c99-4fb6c76f3a91"
[compat]
DataFrames = "0.18"
StatsBase = "0.32, 0.33"
[extras]
DataFrames = "a93c6f00-e57d-5684-b7b6-d8193f3e46c0"
Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
[targets]
test = ["DataFrames", "Test"] If you clone this package, enter it with And if you look at the [[StatsBase]]
deps = ["DataAPI", "DataStructures", "LinearAlgebra", "Missings", "Printf", "Random", "SortingAlgorithms", "SparseArrays", "Statistics"]
git-tree-sha1 = "a6102b1f364befdb05746f386b67c6b7e3262c45"
uuid = "2913bbd2-ae8a-5f71-8c99-4fb6c76f3a91"
version = "0.33.0" Now, open a new Bash session and run the following lines: export JULIA_DEPOT_PATH="$HOME/.julia.temp"
rm -rf $HOME/.julia.temp
export JULIA_PROJECT="@."
rm -rf Foo.jl
git clone https://github.com/aluthge-test/Foo.jl.git Foo.jl
cd Foo.jl
julia You are now in a Julia REPL. Run the following lines in Julia: import Pkg
Pkg.build(; verbose = true)
Pkg.status(; mode = Pkg.PKGMODE_PROJECT)
Pkg.status(; mode = Pkg.PKGMODE_MANIFEST)
Pkg.test() This Bash script and Julia script replicate exactly what the default Travis script for Julia does. Here are the results:
As you can see, So, even though the So, even though your StatsBase = "0.32, 0.33" The test suite ended up using |
Edit: Actually, you should be able to see these results on Julia |
Thank you for the super detailed example! Indeed, this happens on I'm sure the Other alternatives I can think of are:
Either way, it looks like the problem is more challenging than I originally suspected. |
I don't know Pkg internals very well, but isn't the right algorithm something like:
|
See my example above. Even if the resolver gives you
then your test suite might actually use So even if your test suite passes, it doesn't mean that your package is compatible with |
So it seems like there are two orthogonal issues here:
Does that sound correct? |
Yes, it seem pretty important to be able to replicate the environment without having to run the tests. |
As a temporary stopgap, I hacked together a script that (I believe) solves points (1) and (2) above for those using the Run.jl workflow (i.e.
The end result is that the direct dependencies shared between Again, it's a ugly hack, but appears to be working. |
Wouldn't obtaining the actually used versions be quite easy? For example, calling |
When |
In the demo above, all the versions have just freshly been obtained in a CI job. You mean that |
Yeah, it's possible. Because you often have test-only dependencies, which add new constraints to the resolver. The best long-term solution to this issue is JuliaLang/Pkg.jl#2176 |
Fair enough. Thanks |
This is now fixed by the combination of the following:
If you use GitHub Actions for CI, and you use the For example, if your GitHub Actions workflow file uses this line to run the tests...
...then you will get this fix automatically on CI jobs that run on Julia nightly. If you want to double-check that you are getting the fix, you can check the logs. If you have the fix, you will see the following message in the logs:
If you use a different CI provider, or if you use GitHub Actions but you do not use the |
See #298 to track the deployment of this fix across the most commonly used CI providers. |
The tests will then still use the old version and not be effective in knowing if the package actually is compatible with the old version.
The text was updated successfully, but these errors were encountered: