-
Notifications
You must be signed in to change notification settings - Fork 39
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
specify RPATH when linking to libpq #225
Conversation
@@ -145,8 +145,10 @@ async fn collection_task( | |||
struct CollectionTask { | |||
// Channel used to send messages from the agent to the actual task. The task owns the other | |||
// side. | |||
#[allow(dead_code)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bnaecker I added this here because these unused fields were generating clippy warnings, which were treated as errors by the clippy-lint target. It looks like these are unused but I wasn't sure what the right answer was here. It'd be good to remove these allow
directives, one way or another.
I'm not sure why these only generated warnings now. I did update to a newer nightly, and maybe that one has different behavior here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, thanks for cc-ing me. I'm not sure why they never triggered errors before, either, but the updated toolchain seems likely.
These fields are there so that we can communicate with the collection task. For example, updating the interval on which metrics are collected from a producer is accomplished by sending something on the inbox
channel. The join handle might be used if we want to stop collecting metrics from a producer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem like either one is actually used in the code today, though. At least, I think that's what clippy is saying and I couldn't find a case that contradicted it.
After exploring the static linking approach under #234, I've come back around and think this is the most promising direction. The problem I mentioned earlier regarding rustdoc tests is rust-lang/cargo#9895. It appears confirmed as a bug and I believe someone is working on fixing it. With that fixed, this will be a complete solution, even if it's ugly. In the meantime, we can just tell people to set RUSTDOCFLAGS. This is annoying, but at least the failure mode is only that test binaries are broken. In terms of the ugliness: on @cbiffle's suggestion I refactored the build script logic into a new omicron-rpaths package in the same workspace. The remaining ugliness is (1) that nearly every crate in the workspace grows a dummy dependency on pq-sys in order to get its build script metadata, and (2) that same list of crates also grows an identical, tiny build.rs that just calls out to omicron-rpaths, and (3) we're on nightly until 1.56, but we were anyway for "asm". The only thing here that I know needs to be updated before landing this change is the patch in the top level Cargo.toml. We should probably update the "oxide" branch in pq-sys and use that instead. |
Pending the CI checks and the change I mentioned in my last comment, I think this is ready for review. Reviewers: I would suggest:
|
Yes, that’s right. Apologies if I wasn’t clear, but I was trying to say that these are here to support future functionality, so the allow attribute to appease clippy is fine IMO.
… On 14 Sep 2021, at 19:39, David Pacheco ***@***.***> wrote:
@davepacheco commented on this pull request.
In oximeter/oximeter/src/oximeter_server.rs:
> @@ -145,8 +145,10 @@ async fn collection_task(
struct CollectionTask {
// Channel used to send messages from the agent to the actual task. The task owns the other
// side.
+ #[allow(dead_code)]
It doesn't seem like either one is actually used in the code today, though. At least, I think that's what clippy is saying and I couldn't find a case that contradicted it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working in such depth to fix this. Frankly I had no idea this linkage would be so hairy prior to reading your work in #213 , but hopefully this makes life easier when we inevitably need to link against another native library in the future.
@luqmana I'm super interested in your thoughts here - from my point of view, this whole case is a good reason why rpaths should become a first-class citizen in cargo. Do you think there's anything we could/should do to signal boost this to the cargo team?
I still think an "ideal solution" - which would likely require language/cargo changes to be supported - would be one where a "-sys" crate can define explicit rpaths that are transitively picked up by anyone who depends, directly or indirectly, on that "-sys" crate. That said, I understand the concern about "implicit changes to linker-flags", but having a section like:
[build]
transitive_rpath = true
In a workspace seems much more natural than altering the dependency hierarchy to acquire / act on "DEP_" variables.
Regardless, thank you @dap - this solution looks like it'll cover our native linking use cases while protecting us from unexpected missing rpaths for the foreseeable future.
* So we need to emit the linker argument here. How do we know what value to | ||
* use? We take the approach used by most build systems: we use the path where | ||
* the library was found on the build machine. But how do we know where that | ||
* was? *-sys packages have non-trivial mechanisms for locating the desired | ||
* library. We don't want to duplicate those here. Instead, we make use of | ||
* metadata emitted by those build scripts, which shows up as "DEP_*" | ||
* environment variables for us. | ||
* | ||
* **Important note:** In order for us to have metadata for these dependencies, | ||
* we must *directly* depend on them. This may mean adding an otherwise unused | ||
* dependency from the top-level package to the *-sys package. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting hack - I recall reading about the "DEP_" variables and playing around with them, but thought this was a non-starter because of the lack of transitiveness. Good idea just adding the direct dependency!
* expect these to always be set, and if they're not, it's most likely that | ||
* somebody has forgotten to include a required dependency. We want to tell | ||
* them that rather than silently produce unrunnable binaries. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At build time especially, this seems like a reasonable inclination.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @davepacheco for the incredible depth in researching all the possible solutions! I think this is a reasonable approach right now barring any specific support from cargo/rust. Mirroring @smklein sentiments, this got incredibly hairy fast.
I think it'll probably be some time before we can realistically expect a first-class cargo solution there given the limited bandwidth of the cargo team (correct me if I'm wrong @steveklabnik). To really get it accelerated I'd think maybe writing up a pre-RFC and posting to rust-internals would be good. Our own issues and the ones linked above definitely indicate this isn't just a pain point for us. If we can come up with a reasonably scoped surface I'd be happy to help implement/drive it forward.
Thanks @luqmana and @smklein for taking a look and for all the help along the way. I'd love to move the Cargo solution forward, too. There's a lot of good context in the Cargo issue. I'm not sure we have consensus internally on the best approach. It might be helpful to write down the goals and see if we can't find a common solution. I think @smklein wants a package to be able to encapsulate its RPATH requirements (correct me if that's wrong), and other folks (both on the Cargo issue and internally) want to specify this from the top level build. It'd help to write down the use cases where each is desirable. I expect we can come up with a solution where, for folks using |
I spent a while chatting with @rmustacc on this topic today, as I understand it: The advantage of specifying the rpath at a binary / top-level point-of-view is that, at the point a binary is being produced, the target machine should be "known" - as should the target rpath. After all, the I 100% understand the need for top-level targets to be able to determine where rpaths should be uncovered - but it still seems like a bummer that these targets need to basically have hard-coded knowledge about all used native libraries, even if they're the result of a transitive crate dependency. That's the key part I'm wondering about - if the top-level build was responsible for specifying the rpath, but internal "*-sys" crates could emit the required rpaths in some well-known format that could be aggregated somewhere, I think this could accomplish the same goal. Like, right now, the
I could imagine a feature where - similar to how how the The top-level build could opt-in to using this environment variable (... or whatever mechanism cargo prefers), passing it to the linker, but IMO this would have some advantages over what we're being forced to do now:
This would basically change the framing of the problem to: "when you build a top-level target, you see all the rpaths recommended by your dependencies. What do you wanna do with 'em?" With this setup, there would be the option to customize behavior arbitrarily, but also the potential for really concise semantic sugar to say "... yeah, give them all to the linker". |
I think the basic idea makes sense. It's the details I'm not sure about: exactly what information do the *-sys crates expose, how do they get that information, how do they expose it, how do top level crates see that, and how do top level crates use it to control the final RPATH? I still think it's essential that we articulate the two different use cases. Correct me if this is wrong, but I believe the one you're describing is: "my build machine matches my production machine, and I don't want to the top level crate to have to think about the RPATHs for native libraries that are pulled in by transitive dependencies." I think that's a common goal and important to support. I'd like to have a concrete example of what I'll call the "release engineering" case. A simple way to package up Nexus might be to deliver
Now suppose some Nexus saga wants to use
(openssl might be a better example than libpq.) Is that the release engineering case? Is that too contrived? |
I think that's a super reasonable case - and while I want to make the "host = target" case easy, anything proposed should definitely be able to support that case of "the target machine may not look like my own". This is super handwavey, but I think the case you mentioned could basically still be handled effectively if the rpath information emitted by "*-sys" crates was more structured. As an example, suppose the "*-sys" crate could emit:
At a top-level, a resolver in the build.rs file could be used to identify "when you're building this binary, you need Regarding "how does it get exposed up", I dunno. The rust book section on native libraries suggests the "key-value pairs" are basically the go-to. Also, regarding "*-sys packages", the book mentions the following:
The language here implies - to me, at least - that the goal of the "*-sys" packages is to provide a single, centralized location regarding "how do we find the underlying native library" - or in their words:
|
See #213 for context.
I don't think we should land this PR, but I wanted to keep it here for future reference. This was my best attempt at a generalizable pattern for linking to a native library that's not on the runtime linker's default search path. The basic idea is to have the
*-sys
crate that wraps the native library expose metadata that indicates where it found the native library. Then we use this at the "top level" (here in omicron) to pass an appropriate-R
flag to the linker (wrapped in a -W flag because we're really passing it to gcc). This change depends on the "oxide" branch of https://github.com/oxidecomputer/pq-sys, which adds that metadata. (This dependency shows up as a "patch" section in the top level Cargo.toml.)The good things about this approach:
There's a bunch of ugly here:
-R
flag that we want.If we were to go with this approach, we could:
The real killer, though, is that "cargo:rustc-link-arg" is not used when building rustdoc tests. The net result is that builders of omicron still need to set RUSTDOCFLAGS if they want to run the test suite. All this ugliness just prevents them from having to set RUSTFLAGS.