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

adding a check to bootstrap script #36256

Merged
merged 5 commits into from
Sep 10, 2016
Merged

adding a check to bootstrap script #36256

merged 5 commits into from
Sep 10, 2016

Conversation

ducks
Copy link
Contributor

@ducks ducks commented Sep 4, 2016

and a check to the rust config script

refs #36207

first crack at making configure detect nodejs

and a check to the rust config script
@rust-highfive
Copy link
Collaborator

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.

@alexcrichton
Copy link
Member

Thanks for the PR! The nodejs logic in bootstrap.py though seems to never be called? I think this'll want to perhaps live in sanity.rs or close to that where we do the probing. A manually configured nodejs = "...", however, would override whatever we end up probing for.

@ducks
Copy link
Contributor Author

ducks commented Sep 6, 2016

Yeah, I didn't make it as far as actually make it do something yet.

I was reading through the bootstrap readme and was probably ask for some guidance but thanks for the advance advice. :)

@brson
Copy link
Contributor

brson commented Sep 6, 2016

I don't have a solid grasp on how all the build system pieces work these days, but I expected this detection to happin in configure, whereafter bootstrap would read it from config.mk, and in turn pass it to compiletest.

@ducks
Copy link
Contributor Author

ducks commented Sep 6, 2016

Dang, ok. I was a bit off. I think I focused on /src/bootstrap too much and will look at configure more.

@ducks
Copy link
Contributor Author

ducks commented Sep 7, 2016

So I think I made a bit of progress. I am now detecting nodejs back in ./configure.
I then pass it through config.rs when it slurps up the config.mk. It seems to pass the sanity check that I added to sanity.rs because when running make, bootstrap compiles fine. I also did some println!ing and it was showing the correct node path.

I get a bit lost after this. Where to pass it next or how to add a test to compiletest for it or if I need to? Any advice on where to look next?

Thanks in advance! :)

// If a manual nodejs was added to the config,
// of if a nodejs install is detected through config, use it.
if build.config.nodejs.is_some() {
need_cmd("nodejs".as_ref())
Copy link
Member

Choose a reason for hiding this comment

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

Here I think you'll want to do:

if let Some(ref s) = build.config.nodejs {
    need_cmd(s.as_ref());
}

@alexcrichton
Copy link
Member

@bors: r+ 89bc13c

Thanks!

@bors
Copy link
Contributor

bors commented Sep 9, 2016

⌛ Testing commit 89bc13c with merge 48dd4f4...

@bors
Copy link
Contributor

bors commented Sep 9, 2016

💔 Test failed - auto-win-msvc-64-cargotest

@alexcrichton
Copy link
Member

@bors: retry

On Fri, Sep 9, 2016 at 12:40 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-msvc-64-cargotest
https://buildbot.rust-lang.org/builders/auto-win-msvc-64-cargotest/builds/1773


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#36256 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95K5IZjAPqNDpaBJy1ff1gOa0YK6Tks5qobYngaJpZM4J0cs7
.

@bors
Copy link
Contributor

bors commented Sep 9, 2016

⌛ Testing commit 89bc13c with merge 1284081...

bors added a commit that referenced this pull request Sep 9, 2016
…207, r=alexcrichton

adding a check to bootstrap script

and a check to the rust config script

refs #36207

first crack at making configure detect nodejs
@bors
Copy link
Contributor

bors commented Sep 9, 2016

💔 Test failed - auto-win-msvc-64-cargotest

@alexcrichton
Copy link
Member

@bors: retry

On Fri, Sep 9, 2016 at 3:27 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-msvc-64-cargotest
https://buildbot.rust-lang.org/builders/auto-win-msvc-64-cargotest/builds/1775


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#36256 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95EKETXVThkUTgYg6MZtlDKA3edhuks5qod1kgaJpZM4J0cs7
.

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.

5 participants