Workarounds in build.rs for Windows compilation #270

visceralfield · 2019-07-16T21:01:43Z

Usage of where on windows rather than which
Slicing of Extended-Length Paths using regex on windows as a workaround to std::process::Command.current_dir not accepting them

vitiral · 2019-07-16T23:06:16Z

bors try

artifact-app/build.rs

vitiral · 2019-07-16T23:11:52Z

artifact-app/build.rs

+            .unwrap();
+        &BOOK.as_path()
+            .to_str()
+            .expect("Invalid book dir")[shortened.start() .. shortened.end()]


I'm still unsure how this equates to what you want. Does this really work for all of these case?

C:/foo C:/foo/bar //?/C:foo/bar/baz

I think you are trying to remove the leading //?/, but wouldn't you want to slice from book[shortened.start() .. ]?

The fact that windows doesn't accept it's own absolute paths is very concerning, and suggests to me that path_abs (a crate I maintain in ergo) should handle this case. Maybe PathInfo::to_non_absolute() or some such method.

See regex tests here for demonstration: https://regex101.com/r/pOevIF/1
The slice could be changed to [shortened.start .. ], which is more concise and essentially the same since the end of the matched text will always be the end of the string, .+ matching any number of any character - it's a matter of style so it's up to you.

On the topic of the windows API - everything in the windows API is concerning. I think the issue here, however, is likely in std::process::Command. I need to put more time into investigating this, but if the issue is in the standard library here it makes more sense for that to be fixed rather than having to handle strange platform edge cases in your own library.

somehow I missed the .. Makes sense now. Either is fine.

vitiral · 2019-07-16T23:13:30Z

artifact-app/build.rs

@@ -23,10 +23,12 @@ lazy_static! {
 fn check_deps() {
    println!("Checking dependencies");

+    let path_finding_cmd = if cfg!(windows) {"where"} else {"which"};


I think I previously had this, and it didn't work in travis. I think travis runs it in a shell-like env in windows.

I'm wondering.... I've heard that windows now supports bash? Does it make sense to build this in windows bash? What version of windows are you building on?

Maybe we could do not just cfg!(windows) but also key on the version of windows. Thoughts?

There are a few options for "bash" in windows. One is Windows Linux Subsystem, which does give you essentially a true bash shell in windows. This isn't built in though, and requires the system hypervisor. This means that Docker, VirtualBox / other VM environments, and WLS all block each other from running. You can't usually rely on this feature unfortunately - I wish that you could. The other option is the MinGW / Git shell, which is just a program along with a bunch of implementations of common shell utilities. Git shell you can pretty much count on being installed on any windows computer which is git cloning any git repository, it's part of the standard windows git installation. It is used so that git hooks, etc., can run cross-platform. Perhaps this is where Travis runs? It may be possible to enforce running which inside of the git shell on windows to accomplish cross-platform support here.

fun, I didn't realize all of that.

I think it would be good if windows developers could run -- and it also ran in travis. I'm not sure why bors is having issues and travis isn't running. I have to investigate...

visceralfield · 2019-07-17T01:03:39Z

PR updated with code nice-ification

bors · 2019-07-17T01:31:07Z

try

Timed out

vitiral · 2019-07-17T02:51:06Z

never mind, it looks like travis windows nearly passed -- a couple of "serve" tests failed, which I'm not terribly concerned with -- especially if there is a windows user capable of PR's.

Would you mind taking a look at the failures and seeing if you can replicate+fix -- or at least see if they might be related to this PR? I wouldn't be surprised if they aren't related.

vitiral · 2019-07-17T02:51:53Z

Cargo.lock

@@ -1,3 +1,5 @@
+# This file is automatically @generated by Cargo.


I included it reflexively because of this without really inspecting the contents of the change. Yeah, not really needed.

vitiral · 2019-07-17T02:53:47Z

artifact-app/build.rs

+
+    let mdbook_dir = if cfg!(windows) {
+
+        let regex = Regex::new("[a-zA-Z].+").unwrap();


nit: I think i would like this slightly better if it were r"[a-zA-Z]+:.*" because at least I know I'm looking at a windows drive (C:/). However, it's up to you.

vitiral · 2019-07-17T02:55:29Z

artifact-app/build.rs

+
+        let regex = Regex::new("[a-zA-Z].+").unwrap();
+        let dir_as_str = BOOK.as_path().to_str().expect("Invalid book dir");
+        let shortened = regex.find(dir_as_str).unwrap();


just return regex.find(dir_as_str).unwrap().as_str() instead of taking the slice.

https://docs.rs/regex/1.1.9/regex/struct.Match.html#method.as_str

oh, that's a really neat feature.

vitiral · 2019-07-17T02:56:31Z

artifact-app/build.rs

+        &dir_as_str[shortened.start() .. shortened.end()]
+    } else {
+
+        BOOK.as_path().to_str().expect("Invalid book dir")


move dir_as_str up to remove this duplicate as well.

visceralfield · 2019-07-17T13:42:09Z

Alright, cleaned things up a little more. I've ran the failed Travis test cases on my local machine and was able to handle them without issue - I can take a deeper look and maybe uncover a little more this evening.

visceralfield · 2019-07-17T14:39:00Z

Update on the extended length paths issue. I ran the following code expecting passes to pass and fails to fail, but they both passed.

#[cfg(test)]
mod tests {

    use std::process::Command;

    #[test]
    fn passes() {

        let status = Command::new("where")
            .arg("cmd")
            .current_dir("C:\\Users")
            .status()
            .unwrap();

        println!("status: {}", status);
    }

    #[test]
    fn fails() {

        let status = Command::new("where")
            .arg("cmd")
            .current_dir("\\\\?\\C:\\Users")
            .status()
            .unwrap();

        println!("status: {}", status);
    }
}

So I need to go back and do more digging. It is possible that my workaround regex stuff isn't needed, and there is a simpler solution here. I will look into it this evening.

vitiral · 2019-07-22T19:04:25Z

Thanks for all of this.

I strongly support getting this running on windows, but just a heads up that I'm doing a rewrite in python for easier inclusion in build systems (and cross-platform support).

See #271

Joshua Thompson added 3 commits July 16, 2019 14:23

Workarounds to enable building on Windows

734d94b

Fixed accidental commit of tabs

cdfef18

Reverted change done for testing

104306e

visceralfield mentioned this pull request Jul 16, 2019

Building from source on windows #269

Open

bors bot added a commit that referenced this pull request Jul 16, 2019

Try #270:

e538f9a

vitiral reviewed Jul 16, 2019

View reviewed changes

Fix of ugly repetition

09e19b1

vitiral reviewed Jul 17, 2019

View reviewed changes

Prettier still

d10adc3

Clarified PR content

b2eb894

visceralfield closed this by deleting the head repository Mar 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workarounds in build.rs for Windows compilation #270

Workarounds in build.rs for Windows compilation #270

visceralfield commented Jul 16, 2019

vitiral commented Jul 16, 2019

vitiral Jul 16, 2019 •

edited

Loading

visceralfield Jul 17, 2019

visceralfield Jul 17, 2019

vitiral Jul 17, 2019

vitiral Jul 16, 2019

visceralfield Jul 17, 2019

vitiral Jul 17, 2019

visceralfield commented Jul 17, 2019

bors bot commented Jul 17, 2019

vitiral commented Jul 17, 2019

vitiral Jul 17, 2019

visceralfield Jul 17, 2019 •

edited

Loading

vitiral Jul 17, 2019

vitiral Jul 17, 2019

vitiral Jul 17, 2019

visceralfield Jul 17, 2019

vitiral Jul 17, 2019

visceralfield commented Jul 17, 2019

visceralfield commented Jul 17, 2019

vitiral commented Jul 22, 2019

		@@ -1,3 +1,5 @@
		# This file is automatically @generated by Cargo.


		let mdbook_dir = if cfg!(windows) {

		let regex = Regex::new("[a-zA-Z].+").unwrap();

Workarounds in build.rs for Windows compilation #270

Workarounds in build.rs for Windows compilation #270

Conversation

visceralfield commented Jul 16, 2019

vitiral commented Jul 16, 2019

vitiral Jul 16, 2019 • edited Loading

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

visceralfield commented Jul 17, 2019

bors bot commented Jul 17, 2019

try

Timed out

vitiral commented Jul 17, 2019

Choose a reason for hiding this comment

visceralfield Jul 17, 2019 • edited Loading

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

visceralfield commented Jul 17, 2019

visceralfield commented Jul 17, 2019

vitiral commented Jul 22, 2019

vitiral Jul 16, 2019 •

edited

Loading

visceralfield Jul 17, 2019 •

edited

Loading