Updated std::dynamic_lib to use std::path. #23197

aatxe · 2015-03-08T18:59:42Z

std::dynamic_library is currently using std::old_io::Path specifically. This change brings the API in alignment with std::fs::File by having it take std::path::AsPath. The Windows code should work, but I admittedly haven't tried it (I don't have a Windows machine readily available right now).

r? @alexcrichton

rust-highfive · 2015-03-08T18:59:56Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (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. 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 CONTRIBUTING.md for more information.

alexcrichton · 2015-03-09T16:06:49Z

src/libstd/dynamic_lib.rs

@@ -51,8 +52,8 @@ impl DynamicLibrary {

    /// Lazily open a dynamic library. When passed None it gives a
    /// handle to the calling process
-    pub fn open(filename: Option<&Path>) -> Result<DynamicLibrary, String> {
-        let maybe_library = dl::open(filename.map(|path| path.as_vec()));
+    pub fn open<P: AsPath + ?Sized>(filename: Option<&P>) -> Result<DynamicLibrary, String> {


Unfortunately using a type parameter I believe will cause DynamicLibrary::open(None) to fail as there's no way to determine the type of P. Perhaps this could stay as Path for now?

alexcrichton · 2015-03-09T16:07:58Z

While you're at it, would you mine changing DynamicLibrary::search_path as well to return PathBuf instead of old_path::Path? Thanks!

alexcrichton · 2015-03-10T17:41:31Z

@bors: r+ 5b24228

alexcrichton · 2015-03-10T17:52:23Z

@bors: r+ d619afe

bors · 2015-03-10T19:51:25Z

⌛ Testing commit d619afe with merge a61acf6...

bors · 2015-03-10T19:56:40Z

💔 Test failed - auto-win-32-nopt-t

alexcrichton · 2015-03-10T20:32:13Z

@bors: r+ 9ce9ad478d2f032251e728923f753d84639bad1c

bors · 2015-03-11T01:12:59Z

⌛ Testing commit 9ce9ad4 with merge 629557f...

bors · 2015-03-11T01:22:19Z

💔 Test failed - auto-mac-64-nopt-t

aatxe · 2015-03-11T02:27:34Z

I missed some stuff again, but I think I got it this time. Sorry for being bad about this, @alexcrichton. Hopefully this is the last commit.

alexcrichton · 2015-03-11T04:21:36Z

Looks like there's a tidy error (squashes are also nice)

aatxe · 2015-03-11T15:12:29Z

Yup, I accidentally had some trailing whitespace. I fixed that up and I squashed the whole thing into one commit. @alexcrichton

alexcrichton · 2015-03-11T17:36:30Z

@bors: r+ c861692ca6e04d1b54a893218b3c51f1045de362

bors · 2015-03-12T04:30:48Z

⌛ Testing commit c861692 with merge f71ce33...

bors · 2015-03-12T04:48:27Z

💔 Test failed - auto-linux-64-nopt-t

aatxe · 2015-03-12T05:34:28Z

Removed the unused imports that caused the linux build to fail, @alexcrichton.

alexcrichton · 2015-03-12T17:13:48Z

@bors: r+ c80d8fa

bors · 2015-03-12T17:14:09Z

⌛ Testing commit c80d8fa with merge 80093ef...

bors · 2015-03-12T17:39:29Z

💔 Test failed - auto-win-32-nopt-t

Manishearth · 2015-03-12T17:40:40Z

C:/bot/slave/auto-win-32-nopt-t/build/src/libstd\dynamic_lib.rs:277:9: 277:12 error: unused import, #[deny(unused_imports)] on by default
C:/bot/slave/auto-win-32-nopt-t/build/src/libstd\dynamic_lib.rs:277     use str;

aatxe · 2015-03-12T17:42:51Z

Fixed that now, too. Am I missing something? Is there an easy way to test this stuff before committing? Running make check-stage1-std NO_REBUILD=1 NO_BENCH=1 gave no indication of the unused imports (even in the first case with linux).

alexcrichton · 2015-03-12T17:44:00Z

@bors: r+ 267478a

Unfortunately without compiling on windows there's not a great way to do a smoke test of compiling on windows.

aatxe · 2015-03-23T17:47:47Z

@Manishearth @alexcrichton I think I got the rebase correct?

alexcrichton · 2015-03-23T17:52:25Z

src/librustdoc/plugins.rs

@@ -47,7 +48,7 @@ impl PluginManager {
    /// elsewhere, libname.so.
    pub fn load_plugin(&mut self, name: String) {
        let x = self.prefix.join(libname(name));
-        let lib_result = dl::DynamicLibrary::open(Some(&x));
+        let lib_result = dl::DynamicLibrary::open(Some(&x.as_path()));


This call to as_path shouldn't be necessary

aatxe · 2015-03-23T18:06:13Z

@alexcrichton I made all the changes you suggested.

alexcrichton · 2015-03-23T18:14:32Z

src/librustdoc/plugins.rs

@@ -15,6 +15,7 @@ use clean;
 use std::dynamic_lib as dl;
 use serialize::json;
 use std::mem;
+use std::path::AsPath;


This can probably be deleted

aatxe · 2015-03-24T17:30:00Z

@alexcrichton and now I've done the rest of them.

alexcrichton · 2015-03-24T17:46:17Z

@bors: r+ d72691df90f1f522aafb775e77476e4a2edd9511

Thanks!

bors · 2015-03-24T21:21:56Z

⌛ Testing commit d72691d with merge 9451af9...

bors · 2015-03-24T21:27:15Z

💔 Test failed - auto-win-64-nopt-t

aatxe · 2015-03-24T23:24:21Z

@alexcrichton I think I caught all the Windows issues now. (At least, I hope?)

alexcrichton · 2015-03-25T00:06:53Z

@bors: r+ 05f942c

bors · 2015-03-25T05:09:05Z

⌛ Testing commit 05f942c with merge 5f267f2...

bors · 2015-03-25T05:29:15Z

💔 Test failed - auto-linux-64-x-android-t

Manishearth · 2015-03-26T05:09:02Z

/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/dynamic_lib.rs:24:5: 24:7 error: unused import, #[deny(unused_imports)] on by default
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/dynamic_lib.rs:24 use os;
                                                                                                         ^~
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/dynamic_lib.rs:24:5: 24:7 error: use of deprecated item: replaced with std::env APIs, #[deny(deprecated)] on by default
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/dynamic_lib.rs:24 use os;
                                                                                                         ^~
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/dynamic_lib.rs:25:5: 25:8 error: unused import, #[deny(unused_imports)] on by default
/home/rustbuild/src/rust-buildbot/slave/auto-linux-64-x-android-t/build/src/libstd/dynamic_lib.rs:25 use str;
                                                                                                         ^~~
error: aborting due to 3 previous errors
make: *** [x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-

aatxe · 2015-03-27T18:17:05Z

@alexcrichton Added #[cfg(not(target_os = "android"))] on those imports.

alexcrichton · 2015-03-27T20:04:06Z

@bors: r+ alexcrichton@fb3bd98

bors · 2015-03-27T20:04:06Z

📌 Commit 6acf385 has been approved by alexcrichton

alexcrichton · 2015-03-27T20:04:21Z

@bors: r+ fb3bd98d619dd26663df7745e81466fbe8ef2a92

bors · 2015-03-27T20:04:22Z

🙀 fb3bd98d619dd26663df7745e81466fbe8ef2a92 is not a valid commit SHA. Please try again with 6acf385.

alexcrichton · 2015-03-27T20:04:38Z

@bors: r+ 6acf385

@alexcrichton

`std::dynamic_library` is currently using `std::old_io::Path` specifically. This change brings the API in alignment with `std::fs::File` by having it take `std::path::AsPath`. The Windows code should work, but I admittedly haven't tried it (I don't have a Windows machine readily available right now). r? @alexcrichton

rust-highfive assigned alexcrichton Mar 8, 2015

alexcrichton reviewed Mar 9, 2015
View reviewed changes

aatxe changed the title ~~Updated DynamicLibrary::open to use AsPath.~~ Updated std::dynamic_lib to use std::path. Mar 11, 2015

alexcrichton reviewed Mar 23, 2015
View reviewed changes

Updated std::dynamic_lib to use std::path.

6acf385

bors merged commit 6acf385 into rust-lang:master Mar 28, 2015

Updated std::dynamic_lib to use std::path. #23197

Updated std::dynamic_lib to use std::path. #23197

Conversation

aatxe commented Mar 8, 2015

rust-highfive commented Mar 8, 2015

alexcrichton Mar 9, 2015

Choose a reason for hiding this comment

alexcrichton commented Mar 9, 2015

alexcrichton commented Mar 10, 2015

alexcrichton commented Mar 10, 2015

bors commented Mar 10, 2015

bors commented Mar 10, 2015

alexcrichton commented Mar 10, 2015

bors commented Mar 11, 2015

bors commented Mar 11, 2015

aatxe commented Mar 11, 2015

alexcrichton commented Mar 11, 2015

aatxe commented Mar 11, 2015

alexcrichton commented Mar 11, 2015

bors commented Mar 12, 2015

bors commented Mar 12, 2015

aatxe commented Mar 12, 2015

alexcrichton commented Mar 12, 2015

bors commented Mar 12, 2015

bors commented Mar 12, 2015

Manishearth commented Mar 12, 2015

aatxe commented Mar 12, 2015

alexcrichton commented Mar 12, 2015

aatxe commented Mar 23, 2015

alexcrichton Mar 23, 2015

Choose a reason for hiding this comment

aatxe commented Mar 23, 2015

alexcrichton Mar 23, 2015

Choose a reason for hiding this comment

aatxe commented Mar 24, 2015

alexcrichton commented Mar 24, 2015

bors commented Mar 24, 2015

bors commented Mar 24, 2015

aatxe commented Mar 24, 2015

alexcrichton commented Mar 25, 2015

bors commented Mar 25, 2015

bors commented Mar 25, 2015

Manishearth commented Mar 26, 2015

aatxe commented Mar 27, 2015

alexcrichton commented Mar 27, 2015

bors commented Mar 27, 2015

alexcrichton commented Mar 27, 2015

bors commented Mar 27, 2015

alexcrichton commented Mar 27, 2015