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

Arm64 build process for ci #1887

Merged
merged 46 commits into from
Mar 8, 2019
Merged

Arm64 build process for ci #1887

merged 46 commits into from
Mar 8, 2019

Conversation

afinch7
Copy link
Contributor

@afinch7 afinch7 commented Mar 5, 2019

Work in progress build process for arm64 builds in CI.

@afinch7 afinch7 force-pushed the arm-builds-ci branch 4 times, most recently from 053c66e to 7e550fa Compare March 6, 2019 01:33
@afinch7 afinch7 force-pushed the arm-builds-ci branch 2 times, most recently from e59e282 to b40ba3b Compare March 6, 2019 02:28
@afinch7
Copy link
Contributor Author

afinch7 commented Mar 6, 2019

This should work given that it doesn't time out. From testing the cargo build could easily take 30 minutes.

@afinch7
Copy link
Contributor Author

afinch7 commented Mar 8, 2019

Ready for review. I will be unavailable this weekend, so I would recommend holding off merging until monday.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM modulo long lines.

I will wait for @piscisaureus's approval before merging.

.travis.yml Outdated Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
@afinch7 afinch7 changed the title [WIP]: Arm64 build process for ci Arm64 build process for ci Mar 8, 2019
build.rs Outdated Show resolved Hide resolved
build.rs Outdated
@@ -20,8 +20,22 @@ fn main() {
env::var("PROFILE").unwrap()
};

// Equivilent to target arch != host arch
let is_cross_compile = env::var("CARGO_CFG_TARGET_ARCH").unwrap().as_str()
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest let is_cross_compile = env::var("TARGET") != env::var("HOST");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might not have the intended effect. Some targets might vary from host, but will still be compatible with the host. I.E. x86_64-unknown-linux-musl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is_cross_compile might be a bad name for this one.

tools/ts_library_builder/build_library.ts Outdated Show resolved Hide resolved
tools/ts_library_builder/build_library.ts Outdated Show resolved Hide resolved
tools/ts_library_builder/ast_util.ts Outdated Show resolved Hide resolved
Copy link
Contributor Author

@afinch7 afinch7 left a comment

Choose a reason for hiding this comment

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

Did a full merge from master, fixed the formatting on .travis.yml, and made some changes to build.rs.

tools/ts_library_builder/build_library.ts Outdated Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
build.rs Outdated
.collect::<Vec<&str>>()[0];

// If we are not using a standard target
let is_nonstandard_target =
Copy link
Member

Choose a reason for hiding this comment

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

Can you list some example strings here that we might see for "HOST" and "TARGET" ? What is an example of a non-standard target? it's not immediately clear to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HOST, TARGET, and CARGO_CFG_TARGET_ARCH are standard variables provided by cargo and documented here https://doc.rust-lang.org/cargo/reference/environment-variables.html
I might rename this is_not_default_target. The output of rustup target list will tell you what is default for your install of cargo/rustup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I like is_default_target better.

Copy link
Member

Choose a reason for hiding this comment

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

Sure - but some concrete examples are always very valuable when debugging these things - I guess host is "linux-x64" ? or is it "linux-misc-x64"? No one remembers and if it's not listed it will require me to println it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some more detailed comments
Including how to check your default target: rustup target list

Copy link
Member

Choose a reason for hiding this comment

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

It would just be nice to know what these values are concretely on your machine (since this will be their values in 90% of cross-compile cases)

@@ -20,8 +20,27 @@ fn main() {
env::var("PROFILE").unwrap()
};

// Equivalent to target arch != host arch
let is_different_target_arch =
Copy link
Member

Choose a reason for hiding this comment

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

Would be useful to list some examples of what CARGO_CFG_TARGET_ARCH and HOST looks like.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - thank you so much for doing this. I know how annoying it can be waiting for CI over and over. This is a huge contribution : )

@ry ry merged commit 8c7a12d into denoland:master Mar 8, 2019
@kt3k
Copy link
Member

kt3k commented Mar 9, 2022

Note: disabled at c9614d8

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 this pull request may close these issues.

4 participants