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

Install dependencies - Clear up logs - Rebuild pre-built modules #60

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

lag-linaro
Copy link

@lag-linaro lag-linaro commented Nov 29, 2018

Fixes arm build see #54

@geekgonecrazy
Copy link
Contributor

is it necessary to rebuild sharp and grpc here?

@lag-linaro
Copy link
Author

lag-linaro commented Dec 5, 2018

is it necessary to rebuild sharp and grpc here?

Did you read the commit message?

Rebuilding essentially gives you multi-arch support.

@geekgonecrazy
Copy link
Contributor

Ah right! Yes we actually have to do this step in our snap builds also. Also had to do something similar for my alpine PR for our other image.. 🤔 In this case for libc

RocketChat/Rocket.Chat#12548

I wonder if we shouldn't instead have these installed via npm instead of bundled that way always built for the appropriate architecture during npm install instead of having to do this "hack" in multiple ways on multiple distribution methods.

@sampaiodiego is that even possible to move from bundled dependency to installed by end user? Looks like grpc (from google vision package) and sharp

@lag-linaro
Copy link
Author

Yes, if that's possible then it would be better.

Although, it would be good to merge this pull-request in the mean time, as a quick win.

Happy to help you work on the enduring solution too if that's required.

@sampaiodiego
Copy link
Member

is that even possible to move from bundled dependency to installed by end user? Looks like grpc (from google vision package) and sharp

@geekgonecrazy I don't know 😞 the directory bundle/programs/server/npm is added by meteor build command, idk where its content comes from =/

@lag-linaro
Copy link
Author

Penny for your thoughts guys?

@lag-linaro
Copy link
Author

Any updates on this please?

It's been a long time since I submitted this.

@geekgonecrazy
Copy link
Contributor

@lag-linaro sorry for the delay we are actually hung up by #57 please see conversation there. But basically at the moment would do no good to merge because would not be able to be published :(

@lag-linaro
Copy link
Author

Looks like #57 is now closed.

@geekgonecrazy
Copy link
Contributor

Yes the dockerfile has changed some so might require a sync and test to make sure works still. Would gladly merge though if we can get in sync

Lee Jones added 4 commits June 13, 2019 10:04
The 'slim' base variants do not provide adequate tooling to build this
Docker image.  We need to install it before running `npm install` and
ensure proper clean-up once we're finished.

Signed-off-by: Lee Jones <[email protected]>
This ensures each command is printed out before it is executed, which
leads to a much friendlier log for debugging and the like.

Signed-off-by: Lee Jones <[email protected]>
There is no need to print out the name of every file as it is
extracted.  This only serves to cloud the log making it unusable.

Signed-off-by: Lee Jones <[email protected]>
Rocket.Chat comes with some modules pre-compiled.  Unfortunately
only x86_64 modules are supplied.  By rebuilding them locally it
ensures Rocket.Chat Docker images can be built/run on all platforms.

Signed-off-by: Lee Jones <[email protected]>
@lag-linaro
Copy link
Author

Rebased, moved back over to using node:<xxx>-slim and removed debian:jessie-slim hack.

Copy link
Contributor

@geekgonecrazy geekgonecrazy left a comment

Choose a reason for hiding this comment

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

Just putting here to block merging. Rocket.Chat isn't currently compatible with Node.js 8.16.0 which is why we were forced to use a debian base.

So unfortunately we won't be able to merge with that node.js version.

See other PR's here were we discussed the moving over to debian image

#57

If node.js keeps dropping versions off before meteor updates and then we have a chance to test the new meteor version and include in a release... we might not be able to goto the nodejs base image.

@lag-linaro
Copy link
Author

What if I dropped that patch? Is the rest still acceptable?

@lag-linaro
Copy link
Author

Okay Aaron (@geekgonecrazy), I have added the hack back in.

Please re-consider this pull-request.

@lag-linaro
Copy link
Author

@Sing-Li please take a look at this pull-request.

It should provide support for AArch64.

@lag-linaro
Copy link
Author

We've put considerable work into making this work with aarch64.

It would be a real shame to see it go to waste.

Could someone @Sing-Li, @geekgonecrazy please take a look at this?

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Lee Jones seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

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