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

Add the force_latest_compatible_version keyword argument to Pkg.test #2439

Merged
merged 1 commit into from
Mar 30, 2021

Conversation

DilumAluthge
Copy link
Member

@DilumAluthge DilumAluthge commented Mar 19, 2021

Closes #2176
Fixes #2445

Summary

This pull request adds the force_latest_compatible_version::Bool keyword argument to the Pkg.test function.

If force_latest_compatible_version is true, then an error will be thrown if any of the direct dependencies is not at the latest compatible version.

The default value is force_latest_compatible_version = false.

This is an experimental feature. The behavior is subject to change or removal in minor or patch releases of Julia.

How It Works

Here is a summary of how this feature works.

  1. Pkg.test sets up the temporary sandbox project and runs the resolver.
  2. If force_latest_compatible_version is false, we do nothing. If force_latest_compatible_version is true, we assemble the list of all direct dependencies, and then loop over the list; for each direct dependency DepName, we take the following steps:
    1. Inside the temporary sandbox project, we check and see which version of DepName the resolver selected.
    2. We assemble the list of all registered versions of DepName.
    3. We iterate over each version from step (ii), and for each version, we determine whether or not that version is inside the [compat] entry for DepName. At the end of this step, we have a list of all registered versions of DepName that are compatible with the [compat] entry for DepName.
    4. We take the maximum of the list from step (iii). This gives us the latest registered version of DepName that is compatible with the [compat] entry for DepName.
    5. We compute the earliest version number V such that all changes from V to the result of step (iv) are backwards-compatible.
    6. If the answer from step (i) is strictly less than the answer from step (v), we throw an error. Else, we do nothing.
  3. Pkg.test runs the tests/runtests.jl file.

The algorithm for computing step (v) above is as follows. The input is a version number of the form x.y.z.

  • If x > 0, return x.0.0. (This is because, for x > 0, all changes from x.0.0 to x.y.z are backwards-compatible.)
  • Else, if y > 0, return 0.y.0. (This is because, for y > 0, all changes from 0.y.0 to 0.y.z are backwards-compatible.)
  • Else, return 0.0.z.

Motivation

Suppose that you have a [compat] entry that looks like this:

[compat]
Foo = "0.1, 0.2, 0.3"

And suppose that a bot (e.g. CompatHelper, Dependabot, etc.) opens a pull request to edit the above [compat] entry so that it looks like this instead:

[compat]
Foo = "0.1, 0.2, 0.3, 0.4"

When CI runs your package tests on this pull request, you want to make sure that the tests use Foo version 0.4. (If the tests do not use Foo version 0.4, then even if the tests pass, you do not know whether or not your package works with Foo version 0.4.)

However, currently, there is no way to automatically check that your package tests used Foo version 0.4.

With this pull request, you simply set force_latest_compatible_version = true. Then, if your package tests do not use Foo version 0.4, Pkg will throw an error, and therefore your CI job will fail. This will alert you to the fact that you should probably not yet merge the CompatHelper/Dependabot PR.

Future Plans

Note the default value is force_latest_compatible_version = false.

However, for GitHub Actions CI, we will add a feature to julia-actions/julia-runtest that works as follows:

  1. Detect whether or not the current CI job is a pull request job where the pull request was opened by CompatHelper or Dependabot.
  2. If the answer to step 1 is "yes", then set force_latest_compatible_version = true.
  3. If the answer to step 1 is "no", then set force_latest_compatible_version = false.

Related Discussions

  1. Add the force_latest_compat keyword argument to Pkg.test #2176
  2. Set force_latest_compat=true by default on Dependabot/CompatHelper PRs, and set force_latest_compat=false by default otherwise #2234
  3. Don't make PRs to packages that have a dependency that upper bounds the package CompatHelper bumps JuliaRegistries/CompatHelper.jl#160
  4. Add the force_latest_compatible_version input, and add the "auto-detect Dependabot/CompatHelper" functionality julia-actions/julia-runtest#20

@DilumAluthge DilumAluthge force-pushed the dpa/force-latest-compatible-version branch from 8d4a250 to 04a1de8 Compare March 19, 2021 21:30
@DilumAluthge DilumAluthge requested a review from a team as a code owner March 19, 2021 21:30
@DilumAluthge DilumAluthge force-pushed the dpa/force-latest-compatible-version branch from 04a1de8 to 98b9f95 Compare March 19, 2021 21:34
@DilumAluthge DilumAluthge removed the request for review from a team March 19, 2021 21:48
@DilumAluthge DilumAluthge force-pushed the dpa/force-latest-compatible-version branch from 98b9f95 to c17b1ac Compare March 19, 2021 21:56
@DilumAluthge DilumAluthge force-pushed the dpa/force-latest-compatible-version branch 2 times, most recently from 61299f2 to 64546bb Compare March 21, 2021 00:56
@DilumAluthge DilumAluthge force-pushed the dpa/force-latest-compatible-version branch from 5294636 to 147321e Compare March 21, 2021 23:28
@00vareladavid
Copy link
Contributor

I am not super clear on the CompatHelper situation but would it not be sufficient to simply force the latest version for the single package you wish to check e.g. pin Foo@4.0 before you run the tests?

@DilumAluthge DilumAluthge force-pushed the dpa/force-latest-compatible-version branch from fcb4da6 to d037718 Compare March 26, 2021 00:15
@DilumAluthge
Copy link
Member Author

DilumAluthge commented Mar 26, 2021

I am not super clear on the CompatHelper situation but would it not be sufficient to simply force the latest version for the single package you wish to check e.g. pin Foo@4.0 before you run the tests?

So, one of the enhancements that I would like to make in the future is to be able to slightly loosen the requirement from "latest compatible version of your dependency" to "latest compatible breaking family of versions of your dependency".

So, for example, suppose that your package has a [compat] section that currently looks like this:

Foo = "0.1"

And a bot (CompatHelper, Dependabot, etc.) makes a PR to change this to:

Foo = "0.1, 0.2"

Suppose that the list of versions of the Foo.jl package are:

  • 0.1.0
  • 0.2.0
  • 0.2.1
  • 0.3.0

When CI runs your package's test suite on this CompatHelper/Dependabot pull request, we want to make sure that your tests use version 0.2.x of Foo.jl. But, to be honest, we don't actually care if it uses Foo 0.2.0 instead of 0.2.1. According to (Pkg's implementation of) SemVer, if your package is compatible with Foo 0.2.0, then it is also compatible with Foo 0.2.1, because any changes from Foo 0.2.0 -> 0.2.1 must be (by definition) backwards-compatible.

So what we really want is:

  1. Make a list of all registered versions of Foo.jl. This list is:
    • 0.1.0
    • 0.2.0
    • 0.2.1
    • 0.3.0
  2. Filter the list to only include versions that are compatible with our new [compat] entry for Foo. The list now becomes:
    • 0.1.0
    • 0.2.0
    • 0.2.1
  3. Take the maximum of that list. The result of this is 0.2.1.
  4. Compute the earliest version number V such that all changes made from V to the result of step 3 are backwards compatible by SemVer. For this example, the result is 0.2.0.
  5. When the test suite runs, make sure that the version of Foo used by the tests is greater than or equal to the result from step 4. In this example, we make sure that the version of Foo used in the tests is ≥ 0.2.0.

I don't think that we can do this with pin, can we? I.e. we want to "pin" Foo to any version of the form 0.2.x.

I haven't implemented this feature in this PR, because I would like to get this PR reviewed and merged relatively quickly. But I will implement this feature in a future PR. This is really the workflow that we need to support in CompatHelper/Dependabot PRs.


The rule for computing step 4 is:

  • If the input is 0.0.0, return 0.0.0.
  • If the input is 0.0.z, return 0.0.z.
  • Else, if the input if 0.y.z where y is nonzero, return 0.y.0. This is because, if y is nonzero, then any changes from 0.y.0 to 0.y.z must be backwards compatible by SemVer.
  • Else, the input must be x.y.z where x is nonzero. Return x.0.0. This is because, if x is nonzero, then any changes from x.0.0 to x.y.z must be backwards compatible by SemVer.

@DilumAluthge
Copy link
Member Author

The natural follow-up question is: "under what circumstances might my package be compatible with Foo 0.1.0 and Foo 0.2.0 but not Foo 0.2.1"

Suppose that our package's [compat] section looks like this before the CompatHelper/Dependabot PR:

Foo = "0.1"
Bar = "0.8"

And after the CompatHelper/Dependabot PR, it looks like this:

Foo = "0.1, 0.2"
Bar = "0.8"

Suppose that:

  • Foo 0.1.0 is compatible with Bar 0.8.x
  • Foo 0.2.0 is compatible with Bar 0.8.x
  • Foo 0.2.1 is compatible with Bar 0.9.x

Now, our package will be compatible with Foo 0.1.0 and Foo 0.2.0. But it will not be compatible with Foo 0.2.1.

So, when CI runs our package's test suite, all that we need to ensure is that the tests run with Foo 0.2.x. In this case, they will run with Foo 0.2.0. They can't run with Foo 0.2.1 because Foo 0.2.1 requires Bar 0.9.x, but our package only supports Bar 0.8.x.

But, from a CompatHelper/Dependabot point of view, it is perfectly fine that our tests ran with Foo 0.2.0. Because any changes from Foo 0.2.0 to 0.2.1 (or Foo 0.2.0 to Foo 0.2.x) are by definition backwards compatible.

And fundamentally, what we really want to test when CompatHelper/Dependabot makes a PR to change Foo = "0.1" to Foo = "0.1, 0.2" is that our package's tests will pass when the tests are run with Foo 0.2.x for some x.

@00vareladavid
Copy link
Contributor

So, one of the enhancements that I would like to make in the future is to be able to slightly loosen the requirement from "latest compatible version of your dependency" to "latest compatible breaking family of versions of your dependency".

Since CompatHelper knows the new version, it can determine this on its own correct? e.g. if the new version is 0.2.3 it can know that the set of compatible versions is 0.2.x

fundamentally, what we really want to test when CompatHelper/Dependabot makes a PR to change Foo = "0.1" to Foo = "0.1, 0.2" is that our package's tests will pass when the tests are run with Foo 0.2.x for some x.

You can make sure that some 0.2.x is installed with pkg> add Foo@0.2 and you can then pin the package to make sure that Pkg.test preserves it.

It seems like this workflow is already supported? Am I missing something?

@00vareladavid
Copy link
Contributor

FWIW I don't mean to seem like I am necessarily against this 😅 , just that it seems like there is already a way to do it.

@00vareladavid
Copy link
Contributor

If this is the only information we have (i.e. "yes this is a Dependabot PR" or "no this is not a Dependabot PR"), we should still be able to have this functionality.

Ah, I hadn't realized this. In that case it does seem like you would need to read straight from the compat bounds.

Something that could be done (possibly in a future PR) is to take the set of version ranges you compute and add them during the sandbox resolve step. That way you can error out with a resolver trace of why it was not possible (in the case that the resolve step fails). It seems it would give you a better shot of meeting those requirements since you would be explicitly asking the resolver to look for a solution.

@DilumAluthge
Copy link
Member Author

Something that could be done (possibly in a future PR) is to take the set of version ranges you compute and add them during the sandbox resolve step. That way you can error out with a resolver trace of why it was not possible (in the case that the resolve step fails).

Sure, that would be an enhancement that we could make. In the documentation, I've indicated that this feature is experimental, which will allow us to make modification to its behavior.

Since this PR is already pretty big, I'd prefer to get this merged first, and then make this enhancement in a future PR.

src/Operations.jl Outdated Show resolved Hide resolved
src/Operations.jl Outdated Show resolved Hide resolved
src/Operations.jl Outdated Show resolved Hide resolved
src/Operations.jl Outdated Show resolved Hide resolved
Copy link
Contributor

@00vareladavid 00vareladavid left a comment

Choose a reason for hiding this comment

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

I left a few nitpicky suggestions but otherwise looks good to me

@DilumAluthge DilumAluthge force-pushed the dpa/force-latest-compatible-version branch from 6c905a2 to 9406d86 Compare March 30, 2021 00:02
Co-authored-by: Kristoffer Carlsson <kcarlsson89@gmail.com>
Co-authored-by: Ian Butterworth <i.r.butterworth@gmail.com>
Co-authored-by: David Varela <00.varela.david@gmail.com>
@DilumAluthge DilumAluthge force-pushed the dpa/force-latest-compatible-version branch from 9406d86 to 4c441e0 Compare March 30, 2021 01:35
@DilumAluthge
Copy link
Member Author

Thanks @00vareladavid!

@@ -187,10 +187,22 @@ const update = API.up
- `coverage::Bool=false`: enable or disable generation of coverage statistics.
- `julia_args::Union{Cmd, Vector{String}}`: options to be passed the test process.
- `test_args::Union{Cmd, Vector{String}}`: test arguments (`ARGS`) available in the test process.
- `force_latest_compatible_version::Bool=false`: [EXPERIMENTAL] force the latest compatible version of each direct dependency.
- `allow_earlier_backwards_compatible_versions::Bool=false`: [EXPERIMENTAL] allow any version that is backwards-compatible with the latest compatible version of a direct dependency. If `force_latest_compatible_version` is `false`, then the value of `force_latest_compatible_version` has no effect.
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what this means.

(also very long line).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding a test that is pinned to a specific commit of the General registry?
3 participants