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

[Breaking] Remove the "pre-commit hooks" and "update manifests" features #288

Merged
merged 1 commit into from
Apr 11, 2021

Conversation

DilumAluthge
Copy link
Member

@DilumAluthge DilumAluthge commented Apr 11, 2021

Closes #111
Closes #170
Closes #172
Closes #194
Closes #213

Summary

This pull request makes the following changes:

  1. Remove the "pre-commit hook" functionality.
  2. Remove the "update manifests" functionality.
  3. Bump the version number from 1.x.y to 2.0.0, because this is a breaking change.

Motivation

There are multiple reasons why I want to make this change:

  1. There are other (better) options for people that want to update their manifests. I won't make an exhaustive list here, but perhaps someone could make a Discourse post of all of the manifest-updater GitHub Actions.
  2. It is out of scope of the original intention of CompatHelper. The original goal of CompatHelper was simply to bump [compat] entries in the Project.toml file when your dependencies make new breaking releases. In the spirit of "do one thing, and do it well", I'd like to focus on this core mission.
  3. It causes CompatHelper to break if there is a "bad" Manifest.toml file in the repo. This can be quite confusing to users - why should CompatHelper break due to an unrelated Manifest.toml file?

bors bot added a commit that referenced this pull request Apr 11, 2021
@DilumAluthge DilumAluthge force-pushed the dpa/remove-precommit-hooks-and-manifest-updater branch from 0d22ea8 to 1304140 Compare April 11, 2021 13:21
bors bot added a commit that referenced this pull request Apr 11, 2021
@DilumAluthge
Copy link
Member Author

bors merge

@ericphanson
Copy link
Member

Until #282 the suggested workflow wasn’t versioned, so I’d be pretty careful about breaking changes! It seems like this doesn’t break that workflow though, only custom ones, is that right?

@bors bors bot merged commit 9db3fde into master Apr 11, 2021
@bors bors bot deleted the dpa/remove-precommit-hooks-and-manifest-updater branch April 11, 2021 14:08
@DilumAluthge
Copy link
Member Author

Yeah, it will only break custom workflows that pass a custom precommit hook.

If people still need the precommit hook functionality, they will need to use CompatHelper v1.

I'll make a Discourse post asking people to update the CompatHelper workflow files.

@maleadt
Copy link
Contributor

maleadt commented Apr 11, 2021

  • There are other (better) options for people that want to update their manifests. I won't make an exhaustive list here, but perhaps someone could make a Discourse post of all of the manifest-updater GitHub Actions.

  • It is out of scope of the original intention of CompatHelper. The original goal of CompatHelper was simply to bump [compat] entries in the Project.toml file when your dependencies make new breaking releases. In the spirit of "do one thing, and do it well", I'd like to focus on this core mission.

re. these two points: in CUDA.jl I have the Manifest committed, and CI uses it. If CompatHelper now bumps the compat entries without updating the Manifest, I can't be sure that bump is safe because the CI run on that PR used an older Manifest. Sure, there's separate actions that update the Manifest, but that then happens in a different PR, so doesn't help here. Or am I missing something?

@DilumAluthge
Copy link
Member Author

DilumAluthge commented Apr 11, 2021

The short answer is that you can still use CompatHelper v1. Just copy the workflow file from https://github.com/JuliaRegistries/CompatHelper.jl/blob/master/.github/workflows/CompatHelper.yml, and change the line that looks like version = "2" to be version = "1" instead.

The long answer is:

  1. When CompatHelper updated a manifest, it didn't actually check to make sure that the new version of the dependency was used. For example, suppose CompatHelper makes a pull request to change Foo = "0.1" to Foo = "0.1, 0.2". When CompatHelper then updated the manifest, it didn't actually check to make sure that the new manifest used Foo version 0.2.0. So it would be entirely possible for Pkg.update() to result in a manifest that still used Foo version 0.1.0.
  2. Even if you have a manifest checked in to source control, and even if CompatHelper updates the manifest, and even if the manifest includes updated versions of the dependencies, this does not guarantee that your test suite will run with the updated versions of dependencies. For example, suppose that your [compat] entry is Foo = "0.1, 0.2". And suppose that the Manifest.toml file uses Foo version 0.2.0. This does not guarantee that Pkg.test will use Foo version 0.2.0. When you run Pkg.test, Pkg is allowed to re-resolve your dependencies. Therefore, Pkg.test is still allowed to use Foo version 0.1.0. This issue has been solved on GitHub Actions; see Don't make PRs to packages that have a dependency that upper bounds the package CompatHelper bumps #160 (comment) for details of the fix. However, if you don't use GitHub Actions, you won't get this fix. At some point, we should think about adding this fix to Buildkite - basically, we would need to implement Add the force_latest_compatible_version input, and add the "auto-detect Dependabot/CompatHelper" functionality julia-actions/julia-runtest#20 for Buildkite.

@DilumAluthge
Copy link
Member Author

If we can get JuliaLang/Pkg.jl#2504 merged, we can add the "update manifests" feature back to CompatHelper.

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