Fix building libstd on emscripten targets. #31986

ashleysommer · 2016-03-01T11:03:10Z

The main cause of the problem is that libstd/os/mod.rs treats emscripten targets as an alias of linux targets, whereas liblibc treats emscripten targets as musl-compliant, so it gets a slightly different struct stat64 defined.
This commit adds conditional compilation checks to use the correct timestamp format on fs metadata functions in the case of compiling to emscripten targets.

This commit also depends needs https://github.com/ashleysommer/rust/commit/f1575cff2d631e977038fdba3fa3422ba5f8f2fe applied in order to successfully build libstd with emscripten target.

rust-highfive · 2016-03-01T11:03:24Z

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.

ashleysommer · 2016-03-01T11:04:22Z

This is the error I was getting before the changes:

os/linux/fs.rs:19:5: 19:19 error: unresolved import `os::linux::raw`. Could not find `linux` in `os` [E0432]
os/linux/fs.rs:19 use os::linux::raw;
                      ^~~~~~~~~~~~~~
os/linux/fs.rs:19:5: 19:19 help: run `rustc --explain E0432` to see a detailed explanation
os/linux/fs.rs:76:16: 76:65 error: casting `&libc::unix::notbsd::linux::musl::b32::asmjs::stat` as `*const libc::unix::notbsd::linux::musl::b32::asmjs::stat64` is invalid
os/linux/fs.rs:76             &*(self.as_inner().as_inner() as *const libc::stat64
                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
os/linux/fs.rs:105:9: 105:44 error: attempted access of field `st_atime` on type `&libc::unix::notbsd::linux::musl::b32::asmjs::stat`, but no field with that name was found
os/linux/fs.rs:105         self.as_inner().as_inner().st_atime as i64
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
os/linux/fs.rs:108:9: 108:49 error: attempted access of field `st_atime_nsec` on type `&libc::unix::notbsd::linux::musl::b32::asmjs::stat`, but no field with that name was found
os/linux/fs.rs:108         self.as_inner().as_inner().st_atime_nsec as i64
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
os/linux/fs.rs:111:9: 111:44 error: attempted access of field `st_mtime` on type `&libc::unix::notbsd::linux::musl::b32::asmjs::stat`, but no field with that name was found
os/linux/fs.rs:111         self.as_inner().as_inner().st_mtime as i64
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
os/linux/fs.rs:114:9: 114:49 error: attempted access of field `st_mtime_nsec` on type `&libc::unix::notbsd::linux::musl::b32::asmjs::stat`, but no field with that name was found
os/linux/fs.rs:114         self.as_inner().as_inner().st_mtime_nsec as i64
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
os/linux/fs.rs:117:9: 117:44 error: attempted access of field `st_ctime` on type `&libc::unix::notbsd::linux::musl::b32::asmjs::stat`, but no field with that name was found
os/linux/fs.rs:117         self.as_inner().as_inner().st_ctime as i64
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
os/linux/fs.rs:120:9: 120:49 error: attempted access of field `st_ctime_nsec` on type `&libc::unix::notbsd::linux::musl::b32::asmjs::stat`, but no field with that name was found
os/linux/fs.rs:120         self.as_inner().as_inner().st_ctime_nsec as i64
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
sys/unix/fs.rs:153:21: 153:39 error: attempted access of field `st_mtime` on type `libc::unix::notbsd::linux::musl::b32::asmjs::stat`, but no field with that name was found
sys/unix/fs.rs:153             tv_sec: self.stat.st_mtime as libc::time_t,
                                       ^~~~~~~~~~~~~~~~~~
sys/unix/fs.rs:153:31: 153:39 help: did you mean `st_mtim`?
sys/unix/fs.rs:153             tv_sec: self.stat.st_mtime as libc::time_t,
                                                 ^~~~~~~~
sys/unix/fs.rs:154:22: 154:45 error: attempted access of field `st_mtime_nsec` on type `libc::unix::notbsd::linux::musl::b32::asmjs::stat`, but no field with that name was found
sys/unix/fs.rs:154             tv_nsec: self.stat.st_mtime_nsec as libc::c_long,
                                        ^~~~~~~~~~~~~~~~~~~~~~~
sys/unix/fs.rs:154:32: 154:45 help: did you mean `st_mtim`?
sys/unix/fs.rs:154             tv_nsec: self.stat.st_mtime_nsec as libc::c_long,
                                                  ^~~~~~~~~~~~~
sys/unix/fs.rs:160:21: 160:39 error: attempted access of field `st_atime` on type `libc::unix::notbsd::linux::musl::b32::asmjs::stat`, but no field with that name was found
sys/unix/fs.rs:160             tv_sec: self.stat.st_atime as libc::time_t,
                                       ^~~~~~~~~~~~~~~~~~
sys/unix/fs.rs:160:31: 160:39 help: did you mean `st_atim`?
sys/unix/fs.rs:160             tv_sec: self.stat.st_atime as libc::time_t,
                                                 ^~~~~~~~
sys/unix/fs.rs:161:22: 161:45 error: attempted access of field `st_atime_nsec` on type `libc::unix::notbsd::linux::musl::b32::asmjs::stat`, but no field with that name was found
sys/unix/fs.rs:161             tv_nsec: self.stat.st_atime_nsec as libc::c_long,
                                        ^~~~~~~~~~~~~~~~~~~~~~~
sys/unix/fs.rs:161:32: 161:45 help: did you mean `st_atim`?
sys/unix/fs.rs:161             tv_nsec: self.stat.st_atime_nsec as libc::c_long,
                                                  ^~~~~~~~~~~~~
error: aborting due to 11 previous errors
Could not compile `std`.

ashleysommer · 2016-03-01T11:16:04Z

I'm believe libstd used to build fine with emscripten targets, but I think this commit broke it: aa23c98

These new fs metadata functions as defined in libstd/os/linux/fs.rs assume the stat64 struct to be defined as per liblibc/src/unix/notbsd/linux/other/b32/mod.rs but instead liblibc uses liblibc/src/unix/notbsd/linux/musl/b32/asmjs.rs for emscripten targets, which has different fields in its stat64.

tomaka · 2016-03-01T12:12:53Z

src/libstd/os/linux/fs.rs

+#[allow(deprecated)]
+use os::emscripten::raw;
+
+#[cfg(not(target_os = "emscripten"))]


That could be replaced with just use super::raw;.

alexcrichton · 2016-03-01T16:14:12Z

The os::linux::fs module is actually intended to only be compiled on target_os = "linux", could you add a new os::emscripten module for the emscripten port? (that was the original intention)

alexcrichton · 2016-03-01T16:42:33Z

src/libstd/sys/unix/fs.rs

-            tv_sec: self.stat.st_atime as libc::time_t,
-            tv_nsec: self.stat.st_atime_nsec as libc::c_long,
-        }))
+        #[cfg(any(target_env = "musl", target_os = "emscripten"))]


Some of these additions of musl may not be necessary because the standard library is already guaranteed to compile for x86_64-unknown-linux-musl

ashleysommer · 2016-03-01T23:25:22Z

Ok, thanks for the tips. I reverted all the changes in libstd/os/linux/fs.rs and created a new os submodule called emscripten. I copied over the three module files from os/linux/*.rs then removed all the unneeded stuff like checking for x86_64 and checking for MIPS.

I also made some changes in the libstd/os/emscripten/linux/raw.rs file so that the stat struct better matches the stat and stat64 definition in liblibc/src/unix/notbsd/linux/musl/b32/asmjs.rs while maintaining compatibility with the expectations that libstd has about the format of raw::stat.

EDIT: Is it reasonable to assume that when compiling for target_os="emscripten" then target_arch will always be asjms?

alexcrichton · 2016-03-02T00:06:17Z

src/libstd/sys/unix/fs.rs

    }

    pub fn accessed(&self) -> io::Result<SystemTime> {
-        Ok(SystemTime::from(libc::timespec {
-            tv_sec: self.stat.st_atime as libc::time_t,


Could this call self.st_atime() and self.st_atime_nsec()? I think that would help remove the #[cfg]

I've had a look at doing that, but I don't think it is possible.
st_atime() and st_atime_sec() are functions available only on structs that implement libstd::os::emscripted::fs::MetedataExt or libstd::sys::unix::ext::fs::MetadataExt but I believe the FileAttr struct doesn't implement either of those traits.
In this case the FileAttr struct only has access to its own inner stat: stat64 struct.

Oh, right.

In the interest of consistency with other platforms, I think the best solution here would actually be to just modify the libc bindings themselves. Anyone using them manually will require these #[cfg] directives anyway, so we may as well get them all aligned.

Can you elaborate on what you mean here?
Are you saying to change liblibc so that it uses the definitions in unix/notbsd/linux/other/b32 rather than unix/notbsd/linux/musl/b32/asmjs for emscripten targets?
Im not sure if it will be as easy as that, but I can try it out to see how it works.

If we do that, and it works, then most of the changes in this PR will not be required.

Yeah for all other platforms we store the two fields in st_atim differently, one for the seconds and one for the nanoseconds. Looks like we accidentally used timeval here and we should just switch to two separate fields like in other platofrms.

I dont think it was an accident. It was most likely copied from here:
https://github.com/kripken/emscripten/blob/incoming/system/lib/libc/musl/arch/emscripten/bits/stat.h

Yeah, but to mirror how other platforms work and avoid this #[cfg] we expand the struct to an equivalent set of individual fields.

Oh I get it. Yes, I see it the struct will still be byte-compatible and equivalent with individual fields.

Ah I think it's actually getting taken care of in rust-lang/libc#210, once that's merged this can probably just update libc again

Ok, I saw the change was merged, so I targeted libc to that commit, and undone these lines.

ashleysommer · 2016-03-02T02:00:31Z

Should I squash all these commits together?

ashleysommer · 2016-03-07T03:38:14Z

Updated to liblibc e19309c, then took out the extra compile-time cfg checks for emscripten in the FileAttr struct impl. Also rebased to current master, and rebuilt locally and ran all my compilation and usability tests.

alexcrichton · 2016-03-07T07:13:45Z

Thanks! Could you also squash the commits down together as well?

alexcrichton · 2016-03-07T07:13:54Z

src/libstd/sys/unix/fs.rs

@@ -156,7 +160,7 @@ impl FileAttr {
    }

    pub fn accessed(&self) -> io::Result<SystemTime> {
-        Ok(SystemTime::from(libc::timespec {
+        return Ok(SystemTime::from(libc::timespec {


This change can probably be left out for now

Yep, that was an accident. Shouldn't have been in the commit.

Squashed 10 commits: 1) The main cause of the problem is that libstd/os/mod.rs treats emscripten targets as an alias of linux targets, whereas liblibc treats emscripten targets as musl-compliant, so it gets a slightly different struct stat64 defined. This commit adds conditional compilation checks to use the correct timestamp format on fs metadata functions in the case of compiling to emscripten targets. 2) Update previous commit to comply with rust formatting standards. Removed tab characters, remove trailing whitespaces. 3) Move emscripten changes into their own file under libstd/os/emscripten Put libstd/os/linux/fs back to the way it was. 4) Cannot use stat.st_ctim on emscripten to get created time. 5) Remove compile-time conditionals for target_env = musl, it looks like musl builds compile fine already. 6) Undone some formatting changes that are no longer needed, Removed some more target_env="musl" compilation checks that I missed from my previous commit. 7) upgrade to liblibc e19309c, it fixes the differences in the musl stat and stat64 definitions. 8) Undo the compile-time checks to check for emscripten (or musl targets) in the FileAttr struct. No longer needed after updating liblibc to e19309c. 9) Change the MetadataExt implementation of emscripten fs.rs module to match the changes in new liblibc. 10) remove a stray return statement, should have been removed in the previous commit.

alexcrichton · 2016-03-07T07:32:46Z

@bors: r+ 660bbf4

bors · 2016-03-07T12:28:51Z

⌛ Testing commit 660bbf4 with merge d5d838b...

bors · 2016-03-07T13:46:10Z

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

alexcrichton · 2016-03-07T17:43:41Z

@bors: retry

On Mon, Mar 7, 2016 at 5:46 AM, bors [email protected] wrote:

[image: 💔] Test failed - auto-win-gnu-64-nopt-t
http://buildbot.rust-lang.org/builders/auto-win-gnu-64-nopt-t/builds/3341

—
Reply to this email directly or view it on GitHub
#31986 (comment).

bors · 2016-03-07T22:49:38Z

⌛ Testing commit 660bbf4 with merge e06d2ad...

The main cause of the problem is that libstd/os/mod.rs treats emscripten targets as an alias of linux targets, whereas liblibc treats emscripten targets as musl-compliant, so it gets a slightly different struct stat64 defined. This commit adds conditional compilation checks to use the correct timestamp format on fs metadata functions in the case of compiling to emscripten targets. This commit also depends needs https://github.com/ashleysommer/rust/commit/f1575cff2d631e977038fdba3fa3422ba5f8f2fe applied in order to successfully build libstd with emscripten target.

alexcrichton · 2016-03-08T01:04:33Z

@bors: retry force

bors · 2016-03-08T01:04:36Z

⌛ Testing commit 660bbf4 with merge 4352a85...

Fix building libstd on emscripten targets. The main cause of the problem is that libstd/os/mod.rs treats emscripten targets as an alias of linux targets, whereas liblibc treats emscripten targets as musl-compliant, so it gets a slightly different struct stat64 defined. This commit adds conditional compilation checks to use the correct timestamp format on fs metadata functions in the case of compiling to emscripten targets. This commit also depends needs https://github.com/ashleysommer/rust/commit/f1575cff2d631e977038fdba3fa3422ba5f8f2fe applied in order to successfully build libstd with emscripten target.

bors · 2016-03-08T04:06:15Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

…crichton The main cause of the problem is that libstd/os/mod.rs treats emscripten targets as an alias of linux targets, whereas liblibc treats emscripten targets as musl-compliant, so it gets a slightly different struct stat64 defined. This commit adds conditional compilation checks to use the correct timestamp format on fs metadata functions in the case of compiling to emscripten targets. This commit also depends needs https://github.com/ashleysommer/rust/commit/f1575cff2d631e977038fdba3fa3422ba5f8f2fe applied in order to successfully build libstd with emscripten target.

rust-highfive assigned brson Mar 1, 2016

tomaka reviewed Mar 1, 2016
View reviewed changes

alexcrichton reviewed Mar 1, 2016
View reviewed changes

alexcrichton reviewed Mar 2, 2016
View reviewed changes

alexcrichton reviewed Mar 7, 2016
View reviewed changes

bors merged commit 660bbf4 into rust-lang:master Mar 8, 2016

ashleysommer deleted the emscripten_fixes branch March 8, 2016 04:07

gifnksm mentioned this pull request Mar 8, 2016

libstd fails to build on emscripten #31951

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix building libstd on emscripten targets. #31986

Fix building libstd on emscripten targets. #31986

ashleysommer commented Mar 1, 2016

rust-highfive commented Mar 1, 2016

ashleysommer commented Mar 1, 2016

ashleysommer commented Mar 1, 2016

tomaka Mar 1, 2016

alexcrichton commented Mar 1, 2016

alexcrichton Mar 1, 2016

ashleysommer commented Mar 1, 2016

alexcrichton Mar 2, 2016

ashleysommer Mar 2, 2016

alexcrichton Mar 2, 2016

ashleysommer Mar 2, 2016

alexcrichton Mar 3, 2016

ashleysommer Mar 3, 2016

alexcrichton Mar 3, 2016

ashleysommer Mar 4, 2016

alexcrichton Mar 6, 2016

ashleysommer Mar 7, 2016

ashleysommer commented Mar 2, 2016

ashleysommer commented Mar 7, 2016

alexcrichton commented Mar 7, 2016

alexcrichton Mar 7, 2016

ashleysommer Mar 7, 2016

alexcrichton commented Mar 7, 2016

bors commented Mar 7, 2016

bors commented Mar 7, 2016

alexcrichton commented Mar 7, 2016

bors commented Mar 7, 2016

alexcrichton commented Mar 8, 2016

bors commented Mar 8, 2016

bors commented Mar 8, 2016

Fix building libstd on emscripten targets. #31986

Fix building libstd on emscripten targets. #31986

Conversation

ashleysommer commented Mar 1, 2016

rust-highfive commented Mar 1, 2016

ashleysommer commented Mar 1, 2016

ashleysommer commented Mar 1, 2016

Choose a reason for hiding this comment

alexcrichton commented Mar 1, 2016

Choose a reason for hiding this comment

ashleysommer commented Mar 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashleysommer commented Mar 2, 2016

ashleysommer commented Mar 7, 2016

alexcrichton commented Mar 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Mar 7, 2016

bors commented Mar 7, 2016

bors commented Mar 7, 2016

alexcrichton commented Mar 7, 2016

bors commented Mar 7, 2016

alexcrichton commented Mar 8, 2016

bors commented Mar 8, 2016

bors commented Mar 8, 2016