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

Use clang to build nodejs in alpine #281

Closed
wants to merge 1 commit into from
Closed

Use clang to build nodejs in alpine #281

wants to merge 1 commit into from

Conversation

PeterDaveHello
Copy link
Member

Two reasons:

  1. Faster speed
  2. Smaller image

@chorrell
Copy link
Contributor

The test build is failing with:

/usr/bin/../lib/gcc/x86_64-alpine-linux-musl/5.3.0/../../../../include/c++/5.3.0/x86_64-alpine-linux-musl/bits/opt_random.h:33:10: fatal error: 'x86intrin.h' file not found

@PeterDaveHello
Copy link
Member Author

Fixed, I forgot to add clang-dev for the headers, sorry.

@PeterDaveHello
Copy link
Member Author

PeterDaveHello commented Dec 1, 2016

BTW, it took about 4.5 mins with gcc but only 3.5 mins with clang to build v4 on my computer.
The image size is 38.05 MB vs 36.3MB

@chorrell
Copy link
Contributor

chorrell commented Dec 6, 2016

Hi @nodejs/docker

This change seems reasonable to me, but I'm not sure if there are any unintended consequences with switching to Clang when building Node.js under Alpine Linux. Any thoughts or feedback on this?

@PeterDaveHello
Copy link
Member Author

Hi @nodejs/docker ! Can anyone help review this?

Two reasons:
1. Faster speed
2. Smaller image
@retrohacker
Copy link
Contributor

This appears to have been talked about before by the @nodejs/build group with no real concensus given: nodejs/build#1 (comment)

But It looks like clang will be officially supported? nodejs/node#8922
Can't find anywhere this was talked about though, perhaps it happened in a Hangout.

I'm 👍 assuming the Build WG is okay with it 😄

@PeterDaveHello
Copy link
Member Author

Yeah I think it's officially supported:
https://github.com/nodejs/node/blob/master/BUILDING.md#unix--os-x

@jbergstroem
Copy link
Member

clang is supported; alpine not so much - but that's irrelevant here. I'm +1 to this change. Perhaps remove the gcc dependencies?

Another thing to consider: if people were to install node-gyp; would they pull gcc or clang? do we need to readme this?

@PeterDaveHello
Copy link
Member Author

PeterDaveHello commented Dec 10, 2016

@jbergstroem the gcc dependency is just keep there so that we can directly use the library/header dependency, otherwise I'll need to write lots of dependency packages to be installed.

by the apk add --virtual .build-deps and apk del .build-deps, the packages will all be removed except libstdc++, I guess node-gyp wouldn't pull gcc or clang automatically.

@jbergstroem
Copy link
Member

@PeterDaveHello not saying that there would be auto-pulls, rather that people would choose gcc by default over clang because that's what they're used to. Mentioning this use-case in a readme could avoid any fallout as a result.

WRT deps: why not just mention all packages that are required instead of installing extra stuff?

@PeterDaveHello
Copy link
Member Author

I can mention them all once we have conclusion here to decide move to clang :)
For the part of readme, I wonder if that's part of this change? Does this change here affect how they choose their compiler?

@chorrell
Copy link
Contributor

Given the added complexity for users (having to know that they need clang for node-gyp instead of Ggcc), I'm not sure this change is really worth it. I don't think a note in a README is good enough.

We're only saving about 2MB in image file size for an image that's already pretty small already. I guess the shorter build time is a bonus, but it didn't seem that much of a dramatic improvement to me and not something that probably matters to end users who pull this image from the Docker Hub. Also, eventually we want to use a pre-built Node.js binary for Alpine/musl which would more than likely be built with gcc.

After thinking about this, I'm 👎 on this change.

@PeterDaveHello
Copy link
Member Author

So does using clang to compiler make user need to use clang as well when using node-gyp? Just want to figure out the problem.

@chorrell
Copy link
Contributor

I'm assuming that was implied based on what was said in #281 (comment) but it would be good to have that clarified.

@PeterDaveHello PeterDaveHello deleted the alpine-clang branch July 5, 2017 17:04
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.

4 participants