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

Add env var to stop rebuilding dependencies when compile #19

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
9 participants
@ArthurHlt

ArthurHlt commented May 5, 2015

You can see discussions from here: #11 and here #6

It's just to fix the rebuild problem by let's the possibility for the user to no rebuild (by default buildpack will rebuild dependencies).
For no rebuild you have to set an en var: NO_REBUILD to true.

Your nodejs app should be build and install dependencies before pushing to Cloud Foundry you can do it with travis for example.

A good usecase is to try to install https://github.com/cloudfoundry-community/etherpad-lite-cf in offline mode (follow the README) and set NO_REBUILD.

This fix is also to have an option before something better will be made.

@cfdreddbot

This comment has been minimized.

Show comment
Hide comment
@cfdreddbot

cfdreddbot May 5, 2015

Hey ArthurHlt!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you've already signed the CLA.

cfdreddbot commented May 5, 2015

Hey ArthurHlt!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you've already signed the CLA.

@cf-gitbot

This comment has been minimized.

Show comment
Hide comment
@cf-gitbot

cf-gitbot May 5, 2015

Collaborator

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/93895964.

Collaborator

cf-gitbot commented May 5, 2015

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/93895964.

@flavorjones

This comment has been minimized.

Show comment
Hide comment
@flavorjones

flavorjones May 5, 2015

Member

Hey @ArthurHlt,

Thanks so much for sending this contribution!

Ideally I'd like to see some tests around this behavior before we merge it in. I'll schedule some time from the core Buildpacks team to take a look at this PR and possibly put some tests around it.

Thanks again!

Member

flavorjones commented May 5, 2015

Hey @ArthurHlt,

Thanks so much for sending this contribution!

Ideally I'd like to see some tests around this behavior before we merge it in. I'll schedule some time from the core Buildpacks team to take a look at this PR and possibly put some tests around it.

Thanks again!

@ArthurHlt

This comment has been minimized.

Show comment
Hide comment
@ArthurHlt

ArthurHlt May 7, 2015

Yep that's could be good, let me know when it will be done

ArthurHlt commented May 7, 2015

Yep that's could be good, let me know when it will be done

@flavorjones

This comment has been minimized.

Show comment
Hide comment
@flavorjones

flavorjones Jul 7, 2015

Member

Further context from another CF user:

For us (and I suspect the submitter), the reason that this PR is needed is that some npm's will re-fetch dependencies from the Internet when you do 'npm rebuild'. We already had all of the packages present - if you don't issue the rebuild, you don't need to have outbound connectivity configured and we can go on our merry way.

Member

flavorjones commented Jul 7, 2015

Further context from another CF user:

For us (and I suspect the submitter), the reason that this PR is needed is that some npm's will re-fetch dependencies from the Internet when you do 'npm rebuild'. We already had all of the packages present - if you don't issue the rebuild, you don't need to have outbound connectivity configured and we can go on our merry way.

@jtarchie

This comment has been minimized.

Show comment
Hide comment
@jtarchie

jtarchie Jul 15, 2015

Member

@ArthurHlt, could you give us the reproducible instructions that shows the pushing of an app fail with the example application?

Like:

git clone  https://github.com/cloudfoundry-community/etherpad-lite-cf
cd etherpad-lite-cf
npm .....
cf push
cf set-env ....
# etc...
Member

jtarchie commented Jul 15, 2015

@ArthurHlt, could you give us the reproducible instructions that shows the pushing of an app fail with the example application?

Like:

git clone  https://github.com/cloudfoundry-community/etherpad-lite-cf
cd etherpad-lite-cf
npm .....
cf push
cf set-env ....
# etc...
@ArthurHlt

This comment has been minimized.

Show comment
Hide comment
@ArthurHlt

ArthurHlt Jul 15, 2015

Simply follow instructions (Only 5 in installation) from the repo and push the app inside a cloud foundry which have not internet connection, You should see what's going wrong.
you could see, if you prefer, this instructions http://docs.run.pivotal.io/starting/etherpad.html

ArthurHlt commented Jul 15, 2015

Simply follow instructions (Only 5 in installation) from the repo and push the app inside a cloud foundry which have not internet connection, You should see what's going wrong.
you could see, if you prefer, this instructions http://docs.run.pivotal.io/starting/etherpad.html

@flavorjones

This comment has been minimized.

Show comment
Hide comment
@flavorjones

flavorjones Jul 15, 2015

Member

OK, so let me re-state what I understand the issue to be here.

If I've vendored (that is, all my npm dependencies are within my project directory, and getting cf pushed), those vendored files are ignored when the buildpacks runs npm rebuild. Is that correct?

If I'm right, then I'm not convinced that working around this behavior by forcing the developer to set an environment variable. I'd like to do some research into how npm behaves, and if possible have the buildpack just Do The Right Thing™.

If you have knowledge of how npm behaves, I'd love to better understand what our options are here. Could it be as easy as comparing the contents of package.json to what's on disk?

Member

flavorjones commented Jul 15, 2015

OK, so let me re-state what I understand the issue to be here.

If I've vendored (that is, all my npm dependencies are within my project directory, and getting cf pushed), those vendored files are ignored when the buildpacks runs npm rebuild. Is that correct?

If I'm right, then I'm not convinced that working around this behavior by forcing the developer to set an environment variable. I'd like to do some research into how npm behaves, and if possible have the buildpack just Do The Right Thing™.

If you have knowledge of how npm behaves, I'd love to better understand what our options are here. Could it be as easy as comparing the contents of package.json to what's on disk?

@flavorjones

This comment has been minimized.

Show comment
Hide comment
@flavorjones

flavorjones Jul 24, 2015

Member

@ArthurHlt do you have any thoughts on the above?

Member

flavorjones commented Jul 24, 2015

@ArthurHlt do you have any thoughts on the above?

@ArthurHlt

This comment has been minimized.

Show comment
Hide comment
@ArthurHlt

ArthurHlt Jul 25, 2015

Oh I didn't see, for your first question is close to be correct.
in fact it's only module writen in C (e.g: https://github.com/ncb000gt/node.bcrypt.js) which need to be compiled for the current platform will be recompiled by pulling source from github and not use current source code which is vendored.

I think this change is a big chunk for this buildpack and maybe a correction should be made on NPM.

I can't help you so much on understand what actually do NPM, I'm not really an expert and in add, I feel that NPM is a mess to understand.

About the env var, it's just a litle and ephemeral fix, I've made it to give a quick solution for this usecase a better solution should be found.

ArthurHlt commented Jul 25, 2015

Oh I didn't see, for your first question is close to be correct.
in fact it's only module writen in C (e.g: https://github.com/ncb000gt/node.bcrypt.js) which need to be compiled for the current platform will be recompiled by pulling source from github and not use current source code which is vendored.

I think this change is a big chunk for this buildpack and maybe a correction should be made on NPM.

I can't help you so much on understand what actually do NPM, I'm not really an expert and in add, I feel that NPM is a mess to understand.

About the env var, it's just a litle and ephemeral fix, I've made it to give a quick solution for this usecase a better solution should be found.

@Dannyzen

This comment has been minimized.

Show comment
Hide comment
@Dannyzen

Dannyzen Jan 6, 2016

Member

Hi @ArthurHlt would you say this is still relevant or can this be closed?

Member

Dannyzen commented Jan 6, 2016

Hi @ArthurHlt would you say this is still relevant or can this be closed?

@jvshahid

This comment has been minimized.

Show comment
Hide comment
@jvshahid

jvshahid Mar 7, 2016

Member

Closing due to inactivity. @ArthurHlt feel free to comment on the pr if you think it is still relevant.

Member

jvshahid commented Mar 7, 2016

Closing due to inactivity. @ArthurHlt feel free to comment on the pr if you think it is still relevant.

@jvshahid jvshahid closed this Mar 7, 2016

@0x1mason

This comment has been minimized.

Show comment
Hide comment
@0x1mason

0x1mason May 26, 2016

Please reopen this. This is a massive headache when deploying behind firewalls. IIUC, the native modules have already been built, so in many cases there's no point in rebuilding. Rebuilding can also greatly increase deployment time, depending on the number of native modules.

0x1mason commented May 26, 2016

Please reopen this. This is a massive headache when deploying behind firewalls. IIUC, the native modules have already been built, so in many cases there's no point in rebuilding. Rebuilding can also greatly increase deployment time, depending on the number of native modules.

@RochesterinNYC

This comment has been minimized.

Show comment
Hide comment
@RochesterinNYC

RochesterinNYC May 26, 2016

Contributor

@0x1mason do you want to pick up this pull request/change? Pull requests are welcome.

Contributor

RochesterinNYC commented May 26, 2016

@0x1mason do you want to pick up this pull request/change? Pull requests are welcome.

@jtarchie

This comment has been minimized.

Show comment
Hide comment
@jtarchie

jtarchie May 26, 2016

Member

@0x1mason the issue was originally, there were too many use cases for knobs to turn on and off. We had one client ask only to rebuild certain modules because they had not compile all of them. The issue is the npm rebuild does not seem to provide this flexibility.

If this get's reopened, a environment feature flag is not the way to solve it. Because it goes against what I've seen are the best practices of deploying NodeJS applications. If another workflow can be proposed, where npm can still be involved, but ensure that it does the things it needs to do and the things you want it to do, that would be awesome!

Member

jtarchie commented May 26, 2016

@0x1mason the issue was originally, there were too many use cases for knobs to turn on and off. We had one client ask only to rebuild certain modules because they had not compile all of them. The issue is the npm rebuild does not seem to provide this flexibility.

If this get's reopened, a environment feature flag is not the way to solve it. Because it goes against what I've seen are the best practices of deploying NodeJS applications. If another workflow can be proposed, where npm can still be involved, but ensure that it does the things it needs to do and the things you want it to do, that would be awesome!

@0x1mason

This comment has been minimized.

Show comment
Hide comment
@0x1mason

0x1mason May 26, 2016

Sure, I can own it. Do you want that I should submit at new PR?

Here's the problem:

  1. Create a proxied env that locks down npmjs.org, nodejs.org, github.com, etc.
  2. During staging, npm install runs.
  3. Since there is no mechanism to control the npm command, node-gyp rebuild always runs.
  4. Rebuild fails.
  5. The only workaround is to prebuild the native bits and remove the packages from package.json.

@jtarchie I agree that the env var isn't ideal. I'm no CF expert, but from a naive perspective, the Right Way appears to be allowing deployers to customize the npm install cmd that runs during staging.

0x1mason commented May 26, 2016

Sure, I can own it. Do you want that I should submit at new PR?

Here's the problem:

  1. Create a proxied env that locks down npmjs.org, nodejs.org, github.com, etc.
  2. During staging, npm install runs.
  3. Since there is no mechanism to control the npm command, node-gyp rebuild always runs.
  4. Rebuild fails.
  5. The only workaround is to prebuild the native bits and remove the packages from package.json.

@jtarchie I agree that the env var isn't ideal. I'm no CF expert, but from a naive perspective, the Right Way appears to be allowing deployers to customize the npm install cmd that runs during staging.

@0x1mason

This comment has been minimized.

Show comment
Hide comment
@0x1mason

0x1mason commented May 26, 2016

@ArthurHlt

This comment has been minimized.

Show comment
Hide comment
@ArthurHlt

ArthurHlt May 26, 2016

As the original requester i will give you my feelings about that.
First, it was a workaround against this annoying which is coming from npm and not really from this buildpack. When you do npm rebuild it will try to take source code from internet this is the real issue.

I didn't answer back about this PR only cause I think it's something which should be fix on npm (for example add an option to rebuild to not trying download source code from repository for rebuild).

It could be nice to have an ugly (yes it's ugly) workaround for this but it will just postpone the problem.

So, my guess is that we should (I mean the community) report this issue to npm and why not directly fix it with PR even if i think that it might be difficult cause of the way npm took.

ArthurHlt commented May 26, 2016

As the original requester i will give you my feelings about that.
First, it was a workaround against this annoying which is coming from npm and not really from this buildpack. When you do npm rebuild it will try to take source code from internet this is the real issue.

I didn't answer back about this PR only cause I think it's something which should be fix on npm (for example add an option to rebuild to not trying download source code from repository for rebuild).

It could be nice to have an ugly (yes it's ugly) workaround for this but it will just postpone the problem.

So, my guess is that we should (I mean the community) report this issue to npm and why not directly fix it with PR even if i think that it might be difficult cause of the way npm took.

@0x1mason

This comment has been minimized.

Show comment
Hide comment
@0x1mason

0x1mason May 26, 2016

@ArthurHlt I don't think npm is the problem in this case (though I agree there should be more granular control via npmrc). At least for my team, the problem is the node-gyp rebuild that always gets called in dependencies.sh.

It could be that we're talking about 2 different problems, in which case I can create a new issue.

0x1mason commented May 26, 2016

@ArthurHlt I don't think npm is the problem in this case (though I agree there should be more granular control via npmrc). At least for my team, the problem is the node-gyp rebuild that always gets called in dependencies.sh.

It could be that we're talking about 2 different problems, in which case I can create a new issue.

@jtarchie

This comment has been minimized.

Show comment
Hide comment
@jtarchie

jtarchie May 26, 2016

Member

@0x1mason I believe you are talking about the same problem. The original PR tried to disable npm rebuild.

npm does not provide the granular control to do the things that fit these use cases. It is an all or nothing.

We researched this before. It would be great if npm rebuild (or an equivalent command) saw that the binary dependency had already been compiled for the targeted system. It does not.

Member

jtarchie commented May 26, 2016

@0x1mason I believe you are talking about the same problem. The original PR tried to disable npm rebuild.

npm does not provide the granular control to do the things that fit these use cases. It is an all or nothing.

We researched this before. It would be great if npm rebuild (or an equivalent command) saw that the binary dependency had already been compiled for the targeted system. It does not.

@0x1mason

This comment has been minimized.

Show comment
Hide comment
@0x1mason

0x1mason May 26, 2016

@jtarchie The point of rebuild is to rebuild, i.e. if it's already built, then build it again. It explicitly calls clean, configure, and build. It doesn't make sense to have rebuild skip extant binaries. Furthermore, that's behavior in node-gyp, not npm.

What about changing dependencies.sh to call node-gyp build?

0x1mason commented May 26, 2016

@jtarchie The point of rebuild is to rebuild, i.e. if it's already built, then build it again. It explicitly calls clean, configure, and build. It doesn't make sense to have rebuild skip extant binaries. Furthermore, that's behavior in node-gyp, not npm.

What about changing dependencies.sh to call node-gyp build?

@0x1mason

This comment has been minimized.

Show comment
Hide comment
@0x1mason

0x1mason May 27, 2016

On second thought, node-gyp build is a bad idea. My guess is the reasoning behind rebuild is the same reason why *nix package managers run make every time they install native libraries--it guarantees compatibility of the native bits with the target environment. It's a good approach for most cases, just not this one. Skipping rebuild needs to be "opt-in".

My preference would be the ability to customize the cmd, since it puts more control in the hands of deployers. What is the best vehicle for that type of customization? package.json, manifest.yml, Procfile?

0x1mason commented May 27, 2016

On second thought, node-gyp build is a bad idea. My guess is the reasoning behind rebuild is the same reason why *nix package managers run make every time they install native libraries--it guarantees compatibility of the native bits with the target environment. It's a good approach for most cases, just not this one. Skipping rebuild needs to be "opt-in".

My preference would be the ability to customize the cmd, since it puts more control in the hands of deployers. What is the best vehicle for that type of customization? package.json, manifest.yml, Procfile?

@ArthurHlt

This comment has been minimized.

Show comment
Hide comment
@ArthurHlt

ArthurHlt May 27, 2016

@0x1mason Well it's actually what buildpack wants, guarantees compatibility. In many case container used to run your app is under linux 64 OS, you will need to have binaries inside which is compatible with this OS.
That's why rebuild is needed, if you build on your mac for example binaries will be built for this OS and run your app on cloud foundry without rebuild will fail.
So, in my vision it seems necessary.

@jtarchie good idea for verification when npm rebuild, maybe it's not too hard to propose a PR for this on npm or node-gyp. I will take a look.

ArthurHlt commented May 27, 2016

@0x1mason Well it's actually what buildpack wants, guarantees compatibility. In many case container used to run your app is under linux 64 OS, you will need to have binaries inside which is compatible with this OS.
That's why rebuild is needed, if you build on your mac for example binaries will be built for this OS and run your app on cloud foundry without rebuild will fail.
So, in my vision it seems necessary.

@jtarchie good idea for verification when npm rebuild, maybe it's not too hard to propose a PR for this on npm or node-gyp. I will take a look.

@0x1mason

This comment has been minimized.

Show comment
Hide comment
@0x1mason

0x1mason May 27, 2016

@jtarchie Sorry, I glossed over where you said "compiled for the targeted system".

@ArthurHlt Unless I misunderstand, a NPM/node-gyp change would mean you would have to have access to the latest version of NPM to take advantage of the feature. For many people that would mean waiting months or even years before before having access to it.

0x1mason commented May 27, 2016

@jtarchie Sorry, I glossed over where you said "compiled for the targeted system".

@ArthurHlt Unless I misunderstand, a NPM/node-gyp change would mean you would have to have access to the latest version of NPM to take advantage of the feature. For many people that would mean waiting months or even years before before having access to it.

@jtarchie

This comment has been minimized.

Show comment
Hide comment
@jtarchie

jtarchie May 27, 2016

Member

@0x1mason, not if the buildpack had the latest one.

Member

jtarchie commented May 27, 2016

@0x1mason, not if the buildpack had the latest one.

@0x1mason

This comment has been minimized.

Show comment
Hide comment
@0x1mason

0x1mason May 27, 2016

Your approach isn't going to work for us, but I appreciate the dialog. I'll
pursue resolving this internally. Cheers.
On May 27, 2016 5:16 PM, "JT Archie" notifications@github.com wrote:

@0x1mason https://github.com/0x1mason, not if the buildpack had the
latest one.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#19 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AEX_-2L8V4TsjS-vu7PFdkNo7gTPmieNks5qF18lgaJpZM4EQO6h
.

0x1mason commented May 27, 2016

Your approach isn't going to work for us, but I appreciate the dialog. I'll
pursue resolving this internally. Cheers.
On May 27, 2016 5:16 PM, "JT Archie" notifications@github.com wrote:

@0x1mason https://github.com/0x1mason, not if the buildpack had the
latest one.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#19 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AEX_-2L8V4TsjS-vu7PFdkNo7gTPmieNks5qF18lgaJpZM4EQO6h
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment