Skip to content
This repository has been archived by the owner on Oct 25, 2021. It is now read-only.

Workarounds in build.rs for Windows compilation #270

Closed
wants to merge 6 commits into from
Closed

Workarounds in build.rs for Windows compilation #270

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
734d94b
Workarounds to enable building on Windows
Jul 16, 2019
cdfef18
Fixed accidental commit of tabs
Jul 16, 2019
104306e
Reverted change done for testing
Jul 16, 2019
09e19b1
Fix of ugly repetition
Jul 17, 2019
d10adc3
Prettier still
Jul 17, 2019
b2eb894
Clarified PR content
Jul 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 18 additions & 2 deletions artifact-app/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,12 @@ lazy_static! {
fn check_deps() {
println!("Checking dependencies");

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...


let mut missing = Vec::new();

let check_cmd = |m: &mut Vec<_>, cmd: &'static str| {
let is_ok = Command::new("which")
let is_ok = Command::new(path_finding_cmd)
.args(&[cmd])
.output()
.expect("which/where doesn't exist")
Expand All @@ -53,8 +55,22 @@ fn check_deps() {

fn build_mdbook() {
println!("Building the book");

let mdbook_dir = if cfg!(windows) {

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let shortened = regex.find(BOOK.as_path().to_str().expect("Invalid book dir"))
visceralfield marked this conversation as resolved.
Show resolved Hide resolved
.unwrap();
&BOOK.as_path()
.to_str()
.expect("Invalid book dir")[shortened.start() .. shortened.end()]
Copy link
Owner

@vitiral vitiral Jul 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else {

BOOK.as_path().to_str().expect("Invalid book dir")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move dir_as_str up to remove this duplicate as well.

};

let _status = Command::new("mdbook")
.current_dir(BOOK.as_path())
.current_dir(mdbook_dir)
.args(&["build"])
.status()
.unwrap();
Expand Down