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.

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

Build script dylib test issue #6318

Closed
ehuss opened this issue Nov 15, 2018 · 1 comment · Fixed by #6327
Closed

Build script dylib test issue #6318

ehuss opened this issue Nov 15, 2018 · 1 comment · Fixed by #6327

Comments

@ehuss
Copy link
Contributor

ehuss commented Nov 15, 2018

The test build_script_with_dynamic_native_dependency does not seem to be working as it was originally intended. I was going to fix it myself, but there are too many changes that I don't understand. There are two primary issues:

  • This line in the test points to a non-existing path:
    println!("cargo:rustc-link-search=native={}/target/debug/deps",
  • These lines do not have any test coverage (removing add_plugin_deps does not cause any errors):
    // For all plugin dependencies, add their -L paths (now calculated and
    // present in `state`) to the dynamic library load path for the command to
    // execute.
    fn add_plugin_deps(
    rustc: &mut ProcessBuilder,
    build_state: &BuildMap,
    build_scripts: &BuildScripts,
    root_output: &PathBuf,
    ) -> CargoResult<()> {
    let var = util::dylib_path_envvar();
    let search_path = rustc.get_env(var).unwrap_or_default();
    let mut search_path = env::split_paths(&search_path).collect::<Vec<_>>();
    for id in build_scripts.plugins.iter() {
    let key = (id.clone(), Kind::Host);
    let output = build_state
    .get(&key)
    .ok_or_else(|| internal(format!("couldn't find libs for plugin dep {}", id)))?;
    search_path.append(&mut filter_dynamic_search_path(
    output.library_paths.iter(),
    root_output,
    ));
    }
    let search_path = join_paths(&search_path, var)?;
    rustc.env(var, &search_path);
    Ok(())
    }

The history for this test:

It's not clear to me how #3974 was supposed to work. It's also not clear if add_plugin_deps is even needed anymore. I would think all plugin dylibs should already live in the deps dir which is already included in the LD_LIBRARY_PATH, so I don't know why add_plugin_deps exists.

@alexcrichton
Copy link
Member

Hm ok I think that test is basically the test for why add_plugin_deps exist. The addition of a workspace breaks the intention of the test, so those two projects definitely shouldn't be in the same workspace.

As for #1960 I dunno if it's necessary any more, if you can remove plugin = true and the test still works I'd remove it, it looks like one of those things where I just added it to keep something working at the time, and it may have been erroneous and/or no longer necessary

bors added a commit that referenced this issue Nov 18, 2018
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants