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

Look up variables even if they refer to other packages #46

Conversation

Leonidas-from-XIV
Copy link

OpamPackageVar.resolve_switch only looks up variables that are in the current package (and not specifying ~package seems to make no difference), so add code to retrieve the name of the package from the variable, retrieve the package from the installed packages and look up the variable in that package.

This PR:

  1. Fixes the problem of not resolving variables that refer to other packages
  2. Silences the warning about unresolved variables if the variable refers to a package that is not installed.
  3. Keeps the warning if the variable is not referring to a package and undefined OR the package is installed and is missing the definition.

Closes #45.

1. Fixes the problem of not resolving variables that refer to other
   packages
2. Silences the warning about unresolved variables if the variable
   refers to a package that is not installed.
3. Keeps the warning if the variable is not referring to a package and
   undefined OR the package is installed and is missing the definition.

Closes ocaml-opam#45.
@Leonidas-from-XIV Leonidas-from-XIV force-pushed the look-up-variables-from-installed-pkg branch from b28b2a9 to 1f0dcfb Compare December 20, 2022 15:44
@tmattio
Copy link

tmattio commented Jan 10, 2023

Hi! @rjbou pinging you since you're assigned as a reviewer - do you know when you'd have some time to review this or if anyone else would have the bandwidth to have a look?

This is causing unwanted warnings in opam-monorepo: tarides/opam-monorepo#349

@rjbou
Copy link
Contributor

rjbou commented Feb 22, 2023

I don't understand why this need to be done, opam functions already do this resolving. From what I see, the issue comes from

let env t pkg v =
if List.mem v OpamPackageVar.predefined_depends_variables then None
else (
let r = OpamPackageVar.resolve_switch ~package:pkg t.st v in
if r = None then OpamConsole.warning "Unknown variable %S" (OpamVariable.Full.to_string v);
r
)
let filter_deps t pkg f =
let test = OpamPackage.Name.Set.mem (OpamPackage.name pkg) t.test in
f
|> OpamFilter.partial_filter_formula (env t pkg)
|> OpamFilter.filter_deps ~build:true ~post:true ~test ~doc:false ~dev:false ~default:false

From what i understand, OpamPackageVar.resolve_switch is given as argument the package that is looked at, while it should take as argument a package for which it resolves the variable.
For that was, better us OpamPackageVar.resolve, that fully handle package variables, and is more specific to filtering.

@Leonidas-from-XIV
Copy link
Author

@rjbou Thanks for your input.

If I look up the documentation of OpamPackageVar.resolve_switch I get

Resolves global variables within the context of a switch. If a package is specified, "name" and "version" as taken to exclusively resolve to the current package name and version.

Hence I would expect that it would also resolve other packages that are in the switch, but specifying ~package or not didn't seem to make a difference. My impression was that it would look up the values and if I specify ~package it would filter the results to only look up that particular package.

On the other hand, OpamPackageVar.resolve states

Resolves filter variables, including global, switch and package variables ; a map of locally defined variables can be supplied, as well as the opam file of origin, which is used to resolve self-references (implicit ["%{bin}%"] or explicit ["%{_:bin}%"]

Which makes me a bit wary to change, because I don't want global variables to factor into this.

It is well possible that I am using that API wrong, but we began seeing errors in #45 and my hunch of just removing ~package in the original source code didn't help to resolve the variable.

Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request Apr 28, 2023
After a debugging session with @kit-ty-kate as part of fixing
tarides/opam-monorepo#349 and
ocaml-opam/opam-0install-solver#46 we realized
that OPAM doesn't support resolving package variables in `conflicts`
fields (the linter complains about this in `depends` fileds but not in
`conflicts` fields, but the solver never resolves these variables). So
this constraint doesn't do anything in OPAM and it causes issues in the
opam-0install-solver which attempts to resolve them.
@talex5
Copy link
Collaborator

talex5 commented Aug 8, 2024

So, if I understand correctly (from ocaml/opam-repository#23736), the warning comes when a package refers to another package's variables in the conflicts field, which shouldn't be done. So this isn't a problem, and opam-0install-solver is correct to warn about it?

@Leonidas-from-XIV
Copy link
Author

Yes, I think that's a correct reading of the issue. Closing the PR as the issue was solved on the opam-repository side.

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.

Warnings for unknown ocaml variables
4 participants