Link flags are passed to transitive rdeps #346
Conversation
illicitonion
requested review from
acmcarther,
damienmg and
mfarrugi
as code owners
damienmg
left a comment
damienmg
left a comment
There was a problem hiding this comment.
Overall the PR looks fine to me except the usage of tar.
Why tar-ing & detaring?
Passing directory artifact would work just fine. I disagree with the fact that the directory artifact are not supported well in Bazel (there is an issue that the directory might not be created, just like other output artifact, on some remote platform, but it is not a bazel issue per se).
Tar is going to add expensive steps in addition to code complexity.
| let out_dir = Path::new(&out_dir_archive).parent().expect(&format!("out_dir_archive {:?} didn't have a parent", out_dir_archive)).join("out_dir"); | ||
| create_dir_all(&out_dir).expect(&format!("Failed to create directory {:?}", out_dir)); | ||
| write(&out_dir_path_file, out_dir.to_string_lossy().as_bytes()).expect(&format!("Failed to write {:?}", &out_dir_path_file)); | ||
| let out_dir_abs = exec_root.join(&out_dir); |
There was a problem hiding this comment.
Why not use canonicalize?
There was a problem hiding this comment.
See #343 (comment) - resolving symlinks isn't a great thing to implicitly do where what we really want is just an absolute path.
I reverted to using directories :) |
Also, pull out prep_commands for running commands before execing rustc. Also, don't allow both a build_script _and_ an out_dir_tar - one out_dir should be enough for anyone, callers can merge them themselves if needed.
The linkflags need to be propagated transitively to the linker, whereas flags are just for direct dependees.
And absolute paths are redacted into $EXEC_ROOT-relative paths This mirrors Cargo's behaviour of accumulating link flags and passing them to the linker - otherwise there's not much point in sys libraries outputting them. This currently over-adds out_dirs and link flags to all actions, rather than just those that will invoke the linker - we can reduce that if we have a clear indication of what targets will invoke a linker, but I wanted to go with a safe-but-over-invalidating solution rather than picking a bad heuristic.
@illicitonion Would you mind looking over this and seeing if it's sane? It's modifying code that was added in a previous pull request of yours: #346 I use a crate for creating Python extension modules called pyo3. It has code in it like this: ``` #[cfg_attr(windows, link(name = "pythonXY"))] extern "C" { pub fn PyNode_Compile(arg1: *mut _node, arg2: *const c_char) -> *mut PyCodeObject; ``` Its build.rs script locates the relevant library name, and spits out a link flag that maps it to the alias: ``` -lpythonXY:python38 ``` The pyo3 crate compiles successfully, but when I attempt to compile a crate that depends on pyo3 on Windows, the build fails, complaining that the above alias has been provided but there are no mentions of pythonXY in my crate - because the aliases are only in the pyo3 crate. When I compile my crate using cargo instead of bazel, I can see that pyo3's build script link flags are not being passed in when compiling my crate - so the current behaviour of rules_rust does not seem to match what cargo is doing. The change in this PR fixes the issue for me, and hasn't caused any problems here. But your PR specifically mentions transitive deps, so I wonder if I am missing something. Does this change cause a regression in the problem you were originally trying to fix?
3 participants
And absolute paths are redacted into $EXEC_ROOT-relative paths
This mirrors Cargo's behaviour of accumulating link flags and passing
them to the linker - otherwise there's not much point in sys libraries
outputting them.
This currently over-adds out_dirs and link flags to all actions, rather
than just those that will invoke the linker - we can reduce that if we
have a clear indication of what targets will invoke a linker, but I
wanted to go with a safe-but-over-invalidating solution rather than
picking a bad heuristic.
Commits are separately useful to review.
Depends on #343 (the first commit of this PR).
I have a WIP PR which removes both the new usage of system
tarand the existing usages in the PR by building small implementations oftaranduntarin Rust - will publish it shortly, but it's non-trivial enough to prefer doing as a follow-up :)