Add linting check for restricted prefixes

ocurrent / opam-repo-ci

An OCurrent pipeline for testing submissions to opam-repository

Apache License 2.0

20 stars 22 forks source link

Add linting check for restricted prefixes #316

Closed benmandrew closed 3 months ago

benmandrew commented 4 months ago

Fixes #315.

raphael-proust commented 4 months ago

The list of conflict-class (that we want the linter to help keep in sync with the prefixes) is discussed on https://github.com/ocaml/opam-repository/pull/25861#pullrequestreview-2057999557

The discussion is not complete (as time of writing of this comment).

benmandrew commented 4 months ago

The current prefix to conflict class mapping according to my understanding is:

arch- to ocaml-architecture-setting
system- to ocaml-system-setting
host-arch- to ocaml-host-archicture
host-system- to ocaml-host-system
msys2- to msys2-environment

This can easily be changed as the discussion evolves.

benmandrew commented 3 months ago

Recommendations have been integrated and the PR has been rebased onto master; now we're waiting for the finalisation of the discussion on ocaml/opam-repository.

shonfeder commented 3 months ago

The list of conflict-class (that we want the linter to help keep in sync with the prefixes) is discussed on https://github.com/ocaml/opam-repository/pull/25861#pullrequestreview-2057999557

The discussion is not complete (as time of writing of this comment).

@raphael-proust could you confirm whether the prefix/conflict classes here match what you are expect from a maintenance angle?

raphael-proust commented 3 months ago

The prefix (presented as a glob) / conflict classes (presented as a literal string) that were settled on are:

mysys2-* / "msys2-env"
arch-* / "ocaml-arch"
ocaml-env-mingw* / "ocaml-env-mingw"    -- NOTE: the globbing pattern for file names doesn't have a dash
ocaml-env-msvc* / "ocaml-env-msvc"      -- NOTE: same
host-arch-* / "ocaml-host-arch"
host-system-* / "ocaml-host-system"
system-* / "ocaml-system"

shonfeder commented 3 months ago

@punchagan since I've touched this a fair bit and you're looking at the linting stuff anyhow, would mind reviewing this when you have a chance?

punchagan commented 3 months ago

@punchagan since I've touched this a fair bit and you're looking at the linting stuff anyhow, would mind reviewing this when you have a chance?

This looks good to me. I ended up re-ordering and splitting up the commits slightly to split out the refactor commits and the changes that add the new lint checks.

We could move the "analysis" to see if a package is a newly published one to the analysis module, as discussed yesterday. But, it could also come as a separate change.

shonfeder commented 3 months ago

Thank you for the review and the cleanup, @punchagan! I've made a no-op change just to sign the commits so I get the green label XD

We could move the "analysis" to see if a package is a newly published one to the analysis module, as discussed yesterday. But, it could also come as a separate change.

I think that would be the right way, but locating that check in the linting rather the analysis was already inherited in this work. If anything, that should be a followup I think, as it would possibly affect other parts of the pipeline. However, I think we may just postpone this entirely, as we are liable to be replacing all of this with the propotype work soon.