rust: suggest correct dependency when found in Cargo.lock #13448
Conversation
| cargo_wraps = self.wrap_resolver.get_cargo_wraps(crate) | ||
| if not cargo_wraps: | ||
| continue | ||
| raise DependencyException(f'Rust dependency "{name}" not found, ' + |
There was a problem hiding this comment.
DependencyException means this would be fatal, regardless of the value of required...
There was a problem hiding this comment.
Indeed. If we don't find it at this step, there is no chance we find it later, and the error message is thus confusing "Dependency X not found (tried pkgconfig and cmake)".
So I think it's better to make it an hard error instead of a simple warning.
There was a problem hiding this comment.
Someone could make this dep not required, but if it's present in the Cargo.lock file, what would be the use case?
I can check the required flag, if it makes more sense.
There was a problem hiding this comment.
The usual reason for making a dependency not required is to make it a build option so users can decide which components of the project they would like to build.
There was a problem hiding this comment.
Ok. Will check the required flag 👍
There was a problem hiding this comment.
I would probably think to put this below in:
elif required and (dep or i == last):
# This was the last candidate or the dependency has been cached
# as not-found, or cached dependency version does not match,
# otherwise func() would have returned None instead.
raise DependencyException(f'Dependency {self._display_name!r} is required but not found.')as a special-casing of the message you seek to improve. Save cargo_wraps where you currently calculate them, but emit the error message in the existing error condition.
There was a problem hiding this comment.
Sorry, but I'm not getting what this changes concretely.
This PR just tries to add an error message when a user specifiies a Rust dep, and did a mistake in version number used. We specifically want a different error message compared to the normal one.
There was a problem hiding this comment.
It should mean that if you have a C dependency with the same name it gets used as @tristan957 was worried about.
Instead of raising an error during candidate collection if "*-rs" isn't a valid crate dependency we raise an error after all dependency lookups (including both pkg-config and wraps) fail.
There was a problem hiding this comment.
(I currently have a fever. I eyeballed the code and my analysis feels right, but I cannot discount the possibility that what I'm saying is a fever dream in the most literal sense.)
There was a problem hiding this comment.
The problem is that cargo wrap detection is specifically done in a different place than pkg-config/cmake path.
Initially, I tried to implement something more unified, but I realized that cargo integration was modeled quite differently. So I'm not sure how to make this better.
| { | ||
| "stdout": [ | ||
| { | ||
| "line": "test cases/rust/26 cargo lock suggest dependency/meson.build:3:0: ERROR: Rust dependency \"bar-1.0-rs\" not found, but following versions were found using Cargo.lock import: \"bar-1-rs\", \"bar-2-rs\", \"bar-0-rs\", \"bar-0.8-rs\"" |
There was a problem hiding this comment.
I'm not sure whether this really needs a test. All we are testing is the warning message, no? Does that really need an integration test?
There was a problem hiding this comment.
If someone changes the cargo.lock import feature later, it would be useful to notice this. But if you don't see any value in this test, I can remove it too.
pbo-linaro
force-pushed
the
rust_suggest_dep_from_cargolock
branch
from
5ce9af5
to
f288c1e
Compare
|
fixed extra parentheses on the if. |
pbo-linaro
force-pushed
the
rust_suggest_dep_from_cargolock
branch
from
f288c1e
to
69f30dc
Compare
|
Report error only if dependency is required. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| s = name.split('-') | ||
| if len(s) < 3: | ||
| continue | ||
| rs_ending = s.pop() |
Check notice
Code scanning / CodeQL
Unused local variable Note
| if len(s) < 3: | ||
| continue | ||
| rs_ending = s.pop() | ||
| version = s.pop() |
Check notice
Code scanning / CodeQL
Unused local variable Note
mesonbuild/wrap/wrap.py
Outdated
| s = wrap.name.split('-') | ||
| if len(s) < 3: | ||
| continue | ||
| rs_ending = s.pop() |
Check notice
Code scanning / CodeQL
Unused local variable Note
mesonbuild/wrap/wrap.py
Outdated
| if len(s) < 3: | ||
| continue | ||
| rs_ending = s.pop() | ||
| version = s.pop() |
Check notice
Code scanning / CodeQL
Unused local variable Note
pbo-linaro
force-pushed
the
rust_suggest_dep_from_cargolock
branch
from
69f30dc
to
f0fed38
Compare
|
fixed lint issues |
|
@eli-schwartz How can we indicate to meson test infra that the test is supposed to fail? |
pbo-linaro
force-pushed
the
rust_suggest_dep_from_cargolock
branch
2 times, most recently
from
83d2050
to
38d4b72
Compare
|
More idiomatic python. |
pbo-linaro
force-pushed
the
rust_suggest_dep_from_cargolock
branch
from
38d4b72
to
5bc92f9
Compare
|
Use rsplit to extract crate name + update test to have crate with |
|
Thanks for your recommendations @dnicolodi @eli-schwartz. I'm still looking for a way to indicate the test is supposed to fail. If you have suggestion, I would be happy to implement it to fix CI. |
As we refer to crates using API version instead of plain version, it can be confusing to user which is left with a simple "Dependency X not found". Thus, when we detect that a rust dependency is missing, and is not available using a wrap file, if we find something compatible using Cargo.lock import, we just list the correct dependencies names.
pbo-linaro
force-pushed
the
rust_suggest_dep_from_cargolock
branch
from
5bc92f9
to
2272524
Compare
|
Found how to implement a "failing" test, and rebased on top of master. |
|
Test failure does not seem related to this patch (lto issue on cygwin). |
| # Check if we could have a matching Rust crate imported from | ||
| # Cargo.lock, with expected api version. We know there is no | ||
| # wrap-file defined to match this crate. | ||
| if name.endswith('-rs') and required: |
There was a problem hiding this comment.
This is not a good heuristic. What is stopping me from name a C subproject with a -rs suffix? cc @xclaesse
There was a problem hiding this comment.
In case we don't identify a matching cargo wrap, we just continue to next dependency, so the heuristic can't create a false positive.
The only possibility is if someone has a cargo.lock with this exact dependency, and needs a C library named exactly with -rs suffix, which is, IMHO, unlikely to be a real use case.
There was a problem hiding this comment.
This ties into what I said before, I think.
|
Polite ping after two weeks. Review comments have been addressed so far. I still think this error message can be useful for people looking for integrating rust code with meson. |
|
I'm closing this PR for now. If someone is interested to work on this, feel free to reuse the code presented here. |
As we refer to crates using API version instead of plain version, it can be confusing to user which is left with a simple "Dependency X not found".
Thus, when we detect that a rust dependency is missing, and is not available using a wrap file, if we find something compatible using Cargo.lock import, we just list the correct dependencies names.