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

Relax warning 41 for package variables guarded by a :installed filter #5927

Merged
merged 3 commits into from
May 27, 2024

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Apr 18, 2024

The compiler packages contain the following snippet:

depends: [
  "flexdll" {>= "0.42" & os = "win32"}
]
build: [
  [
    "./configure"
    "--with-flexdll=%{flexdll:share}%" {flexdll:installed}
  ]
]

but the use of flexdll:share triggers lint warning 41 because the flexdll dependency is guarded by os. However, this warning is a bit obtuse given the argument is guarded by flexdll:installed (and :installed explicitly doesn't trigger lint warning 41).

This PR alters lint warning 41 so that when it evaluates the list of variables used in commands, it scans the filters for instances of package:installed and then performs a partial evaluation of the filter with that package:installed variable explicitly set to false (but nothing else set in the environment). If the filter reduces to false, then all instances of package:var guarded by the filter are ignored.

The first commit adds a test case showing the present behaviour, then the fix shows two of the packages warned being eliminated.

@dra27
Copy link
Member Author

dra27 commented May 3, 2024

@kit-ty-kate rightly points out a timing issue which concretely invalidates one of the test cases. The filter above should repeat the guard of the dependency, i.e. instead of:

    "--with-flexdll=%{flexdll:share}%" {flexdll:installed}

it should be:

    "--with-flexdll=%{flexdll:share}%" {flexdll:installed & os = "win32"}

this also concretely rules out this line from the test:

  ["%{foo:share}%%{bar:share}%" {bar:installed}] {foo:installed}

since bar is not referred to in the dependency formula in any way.

I'm wondering if this is in fact more related to #5928 - in order to use the variables, we need both that it's unequivocally guarded by :installed (as here) and that the atom appears in some way in either depends or depopt (to enforce the ordering).

Regardless, the test case of bar is a deficiency of this PR, so I'm putting it back to draft status for now.

@dra27
Copy link
Member Author

dra27 commented May 20, 2024

Additional commit added which tweaks the check slightly. Now:

  • foo:share still doesn't trigger warning 41 because it's guarded by foo:installed and foo is referred to in depends
  • Conversely, bar:share now triggers warning 41 because although it's guarded by bar:installed, bar is not referred to at all in depends on depopts
  • baz:share continues to trigger warning 41 because it's neither protected by bar:installed nor referred to in depends/depopts
  • But note that it now triggers warning 41 even for the :installed variable if the package has not been referred to depends/depopts (which I think is better than the present behaviour of always allowing :installed to be referred to, but that's open to further debate!)

dra27 added 3 commits May 27, 2024 11:57
The warnings emitted for guarded-dependency and no-dependency-guarded
are not strictly necessary as they are protected by a filter which
checks :installed (:installed at present never triggers warning 41).

The warning for no-dependency-unguarded is unequivocally correct.
Warning 41 is never triggered for the use of package:installed. Extend
this so that the warning is not triggered for any uses of package:foo
_underneath_ package:installed, i.e.
  "%{package:foo}% {package:installed}
can no longer cause warning 41 on package.
The previous change means that a variable will definitely not be
expanded unless the package has been installed. However, there is a
timing issue which is not desirable - there is no guarantee that if
either no-dependency-guarded or no-dependency-installed-only have been
installed that they will be installed before or after the current
package.

This instability is not desirable, either. The check is therefore
enhanced slightly so that foo:installed can only be used if depends or
depopts in some way mentions the package (rather than doing a full
tautology check on depends for whether foo is installed).
Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

Thanks!

@kit-ty-kate kit-ty-kate merged commit 21305bb into ocaml:master May 27, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants