Properly set dylib search path for doctests

[sgrif]

Fixes #8531.

This issue was introduced by 3fd2814.
Prior to that commit, the rustdoc_process function took other branch
of the if statement being modified by this commit. The flag was
previously called is_host, and rustdoc was run reporting false. In
that commit, the flag was changed to is_rustc_tool, and rustdoc began
reporting true, which resulted in the native directories added by
build scripts no longer being appended to LD_LIBRARY_PATH when running
doctests.

This commit changes the behavior so that we are always appending the
same set of paths, and the only variance is if we are cross compiling or
not. An alternative would be to change rustdoc to always pass
kind: CompileKind::Host, is_rustc_tool: false, but as rustdoc is in
fact a rustc tool, that feels wrong. The commit which introduced the bug
did not include context on why the flag was changed to is_rustc_tool,
so I don't have a sense for if we should just change that flag to mean
something else.

While the test suite previously had an explicit test for the
library path behavior of cargo run, it did not test this for various
test forms. This commit modifies the test to ensure the behavior is
consistent across run, normal tests, and doctests.

[rust-highfive]

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

[weihanglo]

Nice finding! My guessing is that is_rustc_tool tries to separate processes into two use cases: build artifact executions and rustc/rustdoc invocations. However, in reality rustdoc can be executed for both cases (build docs and run doctests). It seems that is_rustc_tool doesn't handle this well. We might need to tell the difference from a higher level such as inside fn rustdoc_process().

Besides, I just found that the doc comment of fill_env is outdated. The doc says it prepares env for running artifacts produced by the build process, but actually it evolved and started building rustc/rustdoc cmd after #3198. This needs some tweaks as well.

[weihanglo]

FWIW, here are pull requests related to the changes of the flag over time:

• false -> true Remove CommandType struct #3198
• true -> false Pass target environment for rustdoc #3205
• false -> true and renamed to is_rustc_tool Support multiple --target flags on the CLI #8167

[sgrif]

Honestly I'm not sure it even makes sense for this flag to exist any more. After this commit, there are only two meaningful things this flag does. The first is that it forces the kind to Host, but given that the kind is only used in this conditional... We could just pass in CompileKind::Host. The second is that it puts the directory for libstd from the host toolchain on the path instead of the build output... But if this is actually required, the test suite never exercises it.

The test suite passes with this more aggressive change https://gist.github.com/sgrif/09cd3144a2f68beca371bcc47631a1ac. I think it's a reasonable change to make if that looks ok to y'all

[weihanglo]

The second is that it puts the directory for libstd from the host toolchain on the path instead of the build output... But if this is actually required, the test suite never exercises it.

I have the impression that rustup does prepend LD_LIBARARY_PATH for every rustc invocation, so supposed we don't need to worry unless there is a use case that runs cargo without rustup existence.

For your aggressive change of the search path order, I am still uncertain about including native_dirs for rustc/rustdoc invocations. Would it hit performance issues? Would it load the wrong library? It seems no harm but maybe @ehuss can shed some light on this 🙏🏾

[ehuss]

Oof. Dynamic library search paths are often brittle and difficult to change. This is a somewhat risky change, but I think we should be able to do something here.

---

so supposed we don't need to worry unless there is a use case that runs cargo without rustup existence.

Cargo does need to work without rustup.

---

The second is that it puts the directory for libstd from the host toolchain on the path instead of the build output... But if this is actually required, the test suite never exercises it.

I've never been entirely clear why it does this. #4006 didn't really explain why it added the corresponding host_dylib_path. I can't envision a scenario where it matters. I think compiler plugins and proc-macros should load just fine, since libstd.so should already be loaded by the compiler. Also, rustc uses rpath these days, and on Windows libstd.so is placed in the bin directory which will be searched first anyways.

On balance, I think it should be safe to just remove sysroot_host_libdir. I can't come up with a scenario where it matters, at least on windows/macos/linux. One issue is that this is most likely to affect plugins, but those are deprecated and I think only Servo was the real holdout. A concern is that nobody is going to test this use case before it hits stable.

---

One concern I had (which I brought up on Zulip) is that this needs to be careful about mixing host and target directories. If there is a shared library of the same name in both directories, the loader may load the wrong one and generate an error.

In general, using shared libraries with proc-macros has some bugs, so hopefully people don't actually use them too often. For example, I just discovered that cargo doc does not set the search path for build script OUT directories, so any proc-macro that tries to load a shared library generated by a build script will fail.

---

One thing I would like to see here is to support -Zdoctest-xcompile if possible. That enables doctests with the --target flag. I think that would require adding paths for both root_output[&CompileKind::Host] (to handle plugins/proc-macros) and root_output[kind] (to handle doctests running on a separate target). That has me very nervous, but I can't see a way around that. The only real alternative I can think of is to separate generating the doctests from running them (maybe with something like --persist-doctests --no-run).

---

Overall, this is really subtle stuff. Some things that weren't immediately clear to me in the existing code:

• native_dirs is only populated after compilation is finished.
• add_plugin_deps handles filling in LD_LIBRARY_PATH during compilation, but that is only used for rustc (it seems rustdoc fails to call it, which could be a problem for a proc-macro with a shared lib from a build script).

If we make any changes here, it might be good to sprinkle some more comments around.

Completely removing is_rustc_tool seems a bit too risky to me, if we can keep the change focused on the associated issue, it might be easier to land this.

[sgrif]

Thanks for the feedback. I'll spend some more time working on this on Monday <3

…

On Fri, Mar 25, 2022 at 5:02 PM Eric Huss ***@***.***> wrote: Oof. Dynamic library search paths are often brittle and difficult to change. This is a somewhat risky change, but I think we should be able to do something here. ------------------------------ so supposed we don't need to worry unless there is a use case that runs cargo without rustup existence. Cargo does need to work without rustup. ------------------------------ The second is that it puts the directory for libstd from the host toolchain on the path instead of the build output... But if this is actually required, the test suite never exercises it. I've never been entirely clear why it does this. #4006 <#4006> didn't really explain why it added the corresponding host_dylib_path. I can't envision a scenario where it matters. I think compiler plugins and proc-macros should load just fine, since libstd.so should already be loaded by the compiler. Also, rustc uses rpath these days, and on Windows libstd.so is placed in the bin directory which will be searched first anyways. On balance, I think it should be safe to just remove sysroot_host_libdir. I can't come up with a scenario where it matters, at least on windows/macos/linux. One issue is that this is most likely to affect plugins, but those are deprecated and I think only Servo was the real holdout. A concern is that nobody is going to test this use case before it hits stable. ------------------------------ One concern I had (which I brought up on Zulip <https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/dynamic.20linkers.20.26.20mixed.20targets/near/276671440>) is that this needs to be careful about mixing host and target directories. If there is a shared library of the same name in both directories, the loader may load the wrong one and generate an error. In general, using shared libraries with proc-macros has some bugs, so hopefully people don't actually use them too often. For example, I just discovered that cargo doc does not set the search path for build script OUT directories, so any proc-macro that tries to load a shared library generated by a build script will fail. ------------------------------ One thing I would like to see here is to support -Zdoctest-xcompile if possible. That enables doctests with the --target flag. I think that would require adding paths for both root_output[&CompileKind::Host] (to handle plugins/proc-macros) and root_output[kind] (to handle doctests running on a separate target). That has me very nervous, but I can't see a way around that. The only real alternative I can think of is to separate generating the doctests from running them (maybe with something like --persist-doctests --no-run). ------------------------------ Overall, this is really subtle stuff. Some things that weren't immediately clear to me in the existing code: - native_dirs is only populated after compilation is finished. - add_plugin_deps handles filling in LD_LIBRARY_PATH during compilation, but that is only used for rustc (it seems rustdoc fails to call it, which could be a problem for a proc-macro with a shared lib from a build script). If we make any changes here, it might be good to sprinkle some more comments around. Completely removing is_rustc_tool seems a bit too risky to me, if we can keep the change focused on the associated issue, it might be easier to land this. — Reply to this email directly, view it on GitHub <#10469 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALVMKYKFPT44GZ5PZOLMFLVBZAW7ANCNFSM5QKP5ORQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

-- Please consider the environment before creating an NFT from this email

[justsmth]

Any updates on this?

[ehuss]

Ping @sgrif, just wondering if you're still interested in working on this?

[sgrif]

If someone else wants to take over I would be fine with it. I left the job where my co-worker was hitting this and am currently on sabbatical

…

On Wed, Jul 13, 2022 at 5:41 PM Eric Huss ***@***.***> wrote: Ping @sgrif <https://github.com/sgrif>, just wondering if you're still interested in working on this? — Reply to this email directly, view it on GitHub <#10469 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALVMKZ4WFEPUEE2HP567V3VT5HZ7ANCNFSM5QKP5ORQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Please consider the environment before creating an NFT from this email

[ehuss]

Ok, thanks for the update, and hope you are enjoying the sabbatical! 😄

I'm going to close this, then. If someone wants to pick it back up, feel free to open a new PR.
Thanks!

[sgrif]

sgtm

…

On Wed, Jul 13, 2022 at 7:05 PM Eric Huss ***@***.***> wrote: Closed #10469 <#10469>. — Reply to this email directly, view it on GitHub <#10469 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALVMKYRFHYEBBZF24OI2WTVT5RTZANCNFSM5QKP5ORQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Please consider the environment before creating an NFT from this email