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

Initial Toolchain support #645

Closed
wants to merge 22 commits into from

Conversation

Globegitter
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Nodejs is downloaded for each platform but does not support toolchains, so does e.g. also not support cross compilation or building of docker images on macOS.

Issue Number: First part of #396

What is the new behavior?

Switches out nodejs to be fetched using platform specific toolchains.

Does this PR introduce a breaking change?

  • [] Yes
  • [] No

Other information

I started working on this a while ago but then got pulled away by trying to finish up other issues and also by starting to work on getting full support also for npm/yarn deps. Functionality wise it has all functionality to get nodejs as a toolchaing and support basic cross-compilation, i.e. building images on macOS that do not have any native dependencies. The platform specfic npm/yarn dependency support is incomplete and as it is turning out to be a more complex I was thinking of pulling this out of this PR and get the basic support in first and focus on npm/yarn specific support in a separate PR.

@Globegitter
Copy link
Contributor Author

This is currently still very much WIP. Will update as soon as it is in a reviewable state.

@Globegitter
Copy link
Contributor Author

Made quite a bit of progress and is functionally very close to be complete and in reviewable state. Should just be a matter of fixing the last platform specific issues from ci and cleaning things up. I will be on holiday next week so probably won't be able to finish this before.

@thesayyn
Copy link
Collaborator

thesayyn commented Apr 9, 2019

I'm looking forward to testing this. Good work

# If tool_path is empty and tool_target is None then there is no local
# node tool, we will just print a nice error message if the user
# attempts to do bazel run
ctx.actions.write(
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is a critical failure then using fail("error message") would be better so execution stops at this point

@gregmagolan
Copy link
Collaborator

gregmagolan commented Apr 15, 2019

Nice. I did a once over and looks like its most of the way there functionally.

A few questions regarding usage:

  • Is node_repositories() still called in the user's workspace?

  • Is the current behavior of preserve_symlinks preserved?

  • How would the different cases look?

    • What would be the behavior of default node_repositories() with toolchains?
    • How would a user select a specific version of node?
    • User wanting to use their locally installed version of node?
    • User wanting to use a version of node that is vendored into their project repo?

@Globegitter
Copy link
Contributor Author

Globegitter commented May 12, 2019

@gregmagolan Thanks for taking a look already and apologies for the delayed response. Yep node_repositories() is still being called and for now should be a 100% backwards compatible (last famous words).

So the default behaviour of node_repositories() is that it will setup the external nodejs repository with the downloaded nodejs binaries with the difference that it will now set-up 4 repositories: nodejs, nodejs_darwin_amd64, nodejs_windows_amd64, nodejs_linux_amd64. One of these repositories is redundant as the host os will match one of the other 3, but since these are created using a macro I can not seem how to avoid this redundancy (though I also do not see a real problem with it). Then we are registering and setting up a toolchain for all these three platforms by calling node_configure.

The vendored node use-case is still supported in the same way with node_repositores() and will get configured equally. Though eventually I would like remove the vendoring functionality from node_repositories() and move it to node_configure directly I have not tested that yet, and not sure I want to focus on sorting that out now already in this PR. Similarly one can now also provide a path to the nodejs binary to use a pre-installed binary on the system. I have note exposed the attribute for this yet, but will do so as I clean things up and add some tests. What would be the best way to test this btw? Would be nice to test some cross-compilation. I guess I could build a docker image on macos? WDYT?

The command to do that btw is: bazel run --platforms=@build_bazel_rules_nodejs//toolchains/node:linux_amd64 //app. It does not work yet with native dependencies, that would be the next step in a separate PR.

@Globegitter
Copy link
Contributor Author

I tested this on my macbook btw and could build a docker container. It is functionally complete, just needs cleaning up, documentation and some tests.

@thesayyn
Copy link
Collaborator

Great, I'm looking forward to this.

@Globegitter Globegitter mentioned this pull request May 28, 2019
12 tasks
@Globegitter
Copy link
Contributor Author

Closed in favour of #827

@Globegitter Globegitter closed this Jun 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants