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

Do not ship default x64 phantomjs binary even for non x64 platform #2683

Closed
fg2it opened this Issue Sep 6, 2015 · 11 comments

Comments

Projects
None yet
3 participants
@fg2it
Contributor

fg2it commented Sep 6, 2015

When creating packages, I noticed that it is always the phantomjs binary shipped in the repo that is included in the packages. The problem is that this binary target x64 platform and I try do build .deb for arm, so, of course, this binary cannot be use.
On x86, during npm install, we should end up with the good binary version somewhere in node_modules/ so this should be the one included.
In my case, due to a misbehave phantomjs npm module (see #376), I have to install phantomjs myself globaly before running npm install, and this should be this global install that should be included in the package.

So, it would be nice that during package creation:

  • no non working binary be included in the package.
  • and, even, better, a correct version of phantomjs be included if it is installed, prefering global install if available or the one install by npm otherwise.
@ranjib

This comment has been minimized.

Show comment
Hide comment
@ranjib

ranjib Sep 6, 2015

i hit this today while building grafana on my Raspberry Pi 2 cluster.
@fg2it did you find any work around?

ranjib commented Sep 6, 2015

i hit this today while building grafana on my Raspberry Pi 2 cluster.
@fg2it did you find any work around?

@ranjib

This comment has been minimized.

Show comment
Hide comment
@ranjib

ranjib Sep 6, 2015

ok. i got it working by grabbing the arm binary from here: https://github.com/aeberhardo/phantomjs-linux-armv6l and replacing it with the one present in node_modules/phantomjs/lib/phantom/bin/phantomjs

ranjib commented Sep 6, 2015

ok. i got it working by grabbing the arm binary from here: https://github.com/aeberhardo/phantomjs-linux-armv6l and replacing it with the one present in node_modules/phantomjs/lib/phantom/bin/phantomjs

@fg2it

This comment has been minimized.

Show comment
Hide comment
@fg2it

fg2it Sep 6, 2015

Contributor

I didn't fix that at the deb level. I replace /usr/share/grafana/vendor/phantomjs/phantomjs with my binary. I am pretty sure that, before package, replacing vendor/phantomjs/phantomjs in your local repo should work.

Contributor

fg2it commented Sep 6, 2015

I didn't fix that at the deb level. I replace /usr/share/grafana/vendor/phantomjs/phantomjs with my binary. I am pretty sure that, before package, replacing vendor/phantomjs/phantomjs in your local repo should work.

@fg2it

This comment has been minimized.

Show comment
Hide comment
@fg2it

fg2it Sep 6, 2015

Contributor

@ranjib not sure that this one will be package in your .deb file. Try replacing vendor/phantomjs/phantomjs.
Anyway, phantomjs seems to be used mainly to create png. So if you don't need that feature, you can force building packages by changing build.go, line 76, with grunt("--force","release").

Contributor

fg2it commented Sep 6, 2015

@ranjib not sure that this one will be package in your .deb file. Try replacing vendor/phantomjs/phantomjs.
Anyway, phantomjs seems to be used mainly to create png. So if you don't need that feature, you can force building packages by changing build.go, line 76, with grunt("--force","release").

@fg2it

This comment has been minimized.

Show comment
Hide comment
@fg2it

fg2it Sep 21, 2015

Contributor

@torkelo Since nobody seems to be in a hurry with this issue, I could try to come up with a PR depending on what you have in mind to fix it. If we stick to my proposal, this should be manageable for me.

Contributor

fg2it commented Sep 21, 2015

@torkelo Since nobody seems to be in a hurry with this issue, I could try to come up with a PR depending on what you have in mind to fix it. If we stick to my proposal, this should be manageable for me.

@torkelo

This comment has been minimized.

Show comment
Hide comment
@torkelo

torkelo Sep 22, 2015

Member

@fg2it yes, I would love a PR based on your proposal (to use binary from npm)

Member

torkelo commented Sep 22, 2015

@fg2it yes, I would love a PR based on your proposal (to use binary from npm)

@fg2it

This comment has been minimized.

Show comment
Hide comment
@fg2it

fg2it Sep 22, 2015

Contributor

@torkelo ok, will try to submit something in the coming days

Contributor

fg2it commented Sep 22, 2015

@torkelo ok, will try to submit something in the coming days

@fg2it

This comment has been minimized.

Show comment
Hide comment
@fg2it

fg2it Sep 23, 2015

Contributor

@torkelo A few thoughts :

  • a first way to proceed is by removing the default phantomjs binary from the repo and having build.go to put the right binary in vendor/phantomjs/phantomjs. It minimizes the changes and locates them in build.go (I am more confortable with go). But, this would lead to an incomplete environment when building with go build . since there would be no default phantomjs binary.
  • a second way is by leaving the default phantomjs binary in the repo and having build.go to copy the right one in temp folder for packaging. Here, things stay pretty much the same but this fix the problem in packages. Still, not much changes, all in build.go.
  • a third way, is to use grunt task. This to have a complete environment, front-end assets have to be build, it would allows to have everything setup when performing go build . or packaging. It still seems preferable to remove the default phantomjs binary from the repo. Unfortunately for me, I am less confortable with grunt tasks, so It would requiere me to go through more things. Changes are probably more numerous and I am unsure if this doesn't bend a bit the current use of grunt tasks.

Let me know what you think about that.

Contributor

fg2it commented Sep 23, 2015

@torkelo A few thoughts :

  • a first way to proceed is by removing the default phantomjs binary from the repo and having build.go to put the right binary in vendor/phantomjs/phantomjs. It minimizes the changes and locates them in build.go (I am more confortable with go). But, this would lead to an incomplete environment when building with go build . since there would be no default phantomjs binary.
  • a second way is by leaving the default phantomjs binary in the repo and having build.go to copy the right one in temp folder for packaging. Here, things stay pretty much the same but this fix the problem in packages. Still, not much changes, all in build.go.
  • a third way, is to use grunt task. This to have a complete environment, front-end assets have to be build, it would allows to have everything setup when performing go build . or packaging. It still seems preferable to remove the default phantomjs binary from the repo. Unfortunately for me, I am less confortable with grunt tasks, so It would requiere me to go through more things. Changes are probably more numerous and I am unsure if this doesn't bend a bit the current use of grunt tasks.

Let me know what you think about that.

@fg2it

This comment has been minimized.

Show comment
Hide comment
@fg2it

fg2it Sep 24, 2015

Contributor

The third way (ie using grunt task), is lighter than I expected. It is a matter of adding a file in tasks/options and adding the new task to default and build. Would have my preference.
If that is ok, remains the question of removing the default phantomjs binary from the repo, which I think would be a good thing : we would not break anything; it would allow a clean build (otherwise getGitSha() from build.go will report a dirty working dir); once the binary would be in place, simply checking the existence of vendor/phantomjs/phantoms would allow to avoid copying the binary for new run of default or build tasks.

Contributor

fg2it commented Sep 24, 2015

The third way (ie using grunt task), is lighter than I expected. It is a matter of adding a file in tasks/options and adding the new task to default and build. Would have my preference.
If that is ok, remains the question of removing the default phantomjs binary from the repo, which I think would be a good thing : we would not break anything; it would allow a clean build (otherwise getGitSha() from build.go will report a dirty working dir); once the binary would be in place, simply checking the existence of vendor/phantomjs/phantoms would allow to avoid copying the binary for new run of default or build tasks.

@torkelo

This comment has been minimized.

Show comment
Hide comment
@torkelo

torkelo Sep 25, 2015

Member

grunt task sound like a good option

Member

torkelo commented Sep 25, 2015

grunt task sound like a good option

@fg2it fg2it referenced this issue Sep 26, 2015

Merged

Phantomjs #2832

@torkelo

This comment has been minimized.

Show comment
Hide comment
@torkelo

torkelo Oct 22, 2015

Member

fixed in #2832

Member

torkelo commented Oct 22, 2015

fixed in #2832

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