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.

Sign up for GitHub

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

Jump to bottom

Attempt to solve #3366, regarding spurious shared library search paths. #3651

Merged
merged 3 commits into from
Feb 8, 2017
Merged

Attempt to solve #3366, regarding spurious shared library search paths. #3651

merged 3 commits into from
Feb 8, 2017

Conversation

pkgw
Copy link
Contributor

@pkgw pkgw commented Feb 5, 2017

This drops native_dirs entries that are not within the target output when modifying the (DY)LD_LIBRARY_PATH environment variable before running programs.

CC #3366

@pkgw
Attempt to solve rust-lang#3366, regarding spurious shared library se…
ff68a27 
…arch paths.

This drops `native_dirs` entries that are not within the target directory when
modifying the (DY)LD_LIBRARY_PATH environment variable before running
programs.
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@pkgw pkgw mentioned this pull request Feb 5, 2017
Closed
@pkgw
Copy link
Contributor Author

pkgw commented Feb 5, 2017

As it stands, this patch breaks the run_with_library_paths test, by design. If the discussion in the relevant issue is correct, that's fine.

@alexcrichton
Copy link
Member

Thanks for the PR! Could you update that test to print out a path that's gets passed through via Cargo? (e.g. print a path in the output dir)

@pkgw
Update the 'run_with_library_paths' test to reflect the new filtering.
8913382 
For the library paths to be passed through, they must reside within the build
output directory.
@alexcrichton
Copy link
Member

Looks good to me, but I think tests are failing on Windows?

@pkgw
Copy link
Contributor Author

pkgw commented Feb 7, 2017

Oh, FFS: the Windows paths contain \ characters which I need to escape when writing them into Rust string literals since they're treated as escape sequences. Is there some kind of utility function that will do that for me?

@alexcrichton
Copy link
Member

I think maybe raw/literal strings in Rust may do the trick?

@pkgw
Attempt to fix the run_with_library_paths test for Windows.
d545a9f 
I was just pasting the build directories into Rust string literals, so Windows
paths with backslashes were being interpreted as having unknown string
escapes. Raw string guards should fix this for all but the most pathological
of build directories.
@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Feb 7, 2017

📌 Commit d545a9f has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Feb 7, 2017

⌛ Testing commit d545a9f with merge fa1b12a...

bors added a commit that referenced this pull request Feb 7, 2017
@bors
Auto merge of #3651 - pkgw:pr-issue-3366, r=alexcrichton
fa1b12a 
Attempt to solve #3366, regarding spurious shared library search paths.

This drops `native_dirs` entries that are not within the target output when modifying the (DY)LD_LIBRARY_PATH environment variable before running programs.

CC #3366
@bors
Copy link
Contributor

bors commented Feb 8, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing fa1b12a to master...

@bors bors merged commit d545a9f into rust-lang:master Feb 8, 2017
@nox nox mentioned this pull request Mar 9, 2017
Closed
@nox
Copy link
Contributor

nox commented Mar 9, 2017

@metajack @aturon This makes Servo fail to run unit tests on our windows-gnu-dev buildbot machine in servo/servo#15852.

@retep998
Copy link
Member

retep998 commented Mar 9, 2017 •
edited
Loading

Any dynamic library that has to be installed manually by the user on Windows will end up with DLLs that are not in PATH normally, yet because the build script specifies the directory where they are cargo would add them in PATH when running binaries. Therefore this change is likely a breaking change for a lot of projects that rely on external libraries. I strongly believe this change should be reverted on Windows.

@alexcrichton alexcrichton mentioned this pull request Mar 17, 2017
Closed
nbigaouette-eai added a commit to nbigaouette-eai/rust that referenced this pull request Mar 17, 2017
@nbigaouette-eai
Fix running under nightly: copy prebuilt libtensorflow.so to OUT_DIR
dc5d263 
A change in cargo nightly (merged on February 7th 2017) changed how
dynamic libraries are loaded. This prevented the `tensorflow-sys` crate's
tests to run.

To fix, simply copy the `libtensorflow.so` pre-built library to `OUT_DIR`
and change `cargo:rustc-link-search=` path so cargo can find it when
running the tests.

See:
* issue: tensorflow#71
* cargo's pull request: rust-lang/cargo#3651
@nbigaouette-eai nbigaouette-eai mentioned this pull request Mar 17, 2017
Merged
nbigaouette-eai added a commit to nbigaouette-eai/rust that referenced this pull request Mar 17, 2017
@nbigaouette-eai
Fix running under nightly: copy prebuilt libtensorflow.so to OUT_DIR
b6bf663 
A change in cargo nightly (merged on February 7th 2017) changed how
dynamic libraries are loaded. This prevented the `tensorflow-sys` crate's
tests to run.

To fix, simply copy the `libtensorflow.so` pre-built library to `OUT_DIR`
and change `cargo:rustc-link-search=` path so cargo can find it when
running the tests.

See:
* issue: tensorflow#71
* cargo's pull request: rust-lang/cargo#3651
mcgoo added a commit to mcgoo/vcpkg-rs that referenced this pull request Apr 28, 2017
@mcgoo
copy dlls to OUT_DIR
fdab8ec 
work around new cargo runtime path changes rust-lang/cargo#3651
@alexcrichton alexcrichton mentioned this pull request May 15, 2017
Open
@ehuss ehuss mentioned this pull request Nov 18, 2018
Merged
bors added a commit that referenced this pull request Nov 18, 2018
@bors
Auto merge of #6327 - ehuss:add_plugin_deps-tests, r=alexcrichton
3468918 
Fix add_plugin_deps-related tests.

These tests were modified in #3974 in such a way that they stopped testing the `add_plugin_deps` code path. The tests can't be directly reverted because #3651 changed it so that dylib paths must be within the root output directory. I compromised by just copying the dylib into `$OUT_DIR`.

Closes #6318.
@ehuss ehuss added this to the 1.17.0 milestone Feb 6, 2022
@pkgw pkgw deleted the pr-issue-3366 branch January 20, 2023 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@brson brson

Labels
None yet
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.
8 participants
@pkgw @rust-highfive @alexcrichton @bors @nox @retep998 @ehuss @brson