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

Adds a 'make charts' makefile target to make it more obvious how to build these. #3306

Merged
merged 5 commits into from
Dec 12, 2016

Conversation

dannon
Copy link
Member

@dannon dannon commented Dec 12, 2016

Not invoked automatically though, since these should very
rarely conflict and we don't want to have version build difference
conflicts (harmless) all the time.

@nsoranzo, @guerler, does this handle it?

build these.  Not invoked automatically though, since these should very
rarely conflict and we don't want to have version build difference
conflicts (harmless) all the time.
@nsoranzo
Copy link
Member

Fails for me with:

$ make charts
cd client && npm install
npm WARN install Couldn't install optional dependency: Unsupported
client/node_modules/webpack/bin/webpack.js -p --config config/plugins/visualizations/charts/webpack.config.js
module.js:340
    throw err;
    ^

Error: Cannot find module 'webpack'
    at Function.Module._resolveFilename (module.js:338:15)
    at Function.Module._load (module.js:289:25)
    at Module.require (module.js:366:17)
    at require (module.js:385:17)
    at Object.<anonymous> (/usr/users/ga002/soranzon/software/galaxyproject_galaxy/config/plugins/visualizations/charts/webpack.config.js:1:77)
    at Module._compile (module.js:425:26)
    at Object.Module._extensions..js (module.js:432:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:313:12)
    at Module.require (module.js:366:17)
make: *** [charts] Error 1

@dannon
Copy link
Member Author

dannon commented Dec 12, 2016

@nsoranzo Weird. Does just the precursor step, make npm-deps work for you? (I think it likely does, but it's failing on the subsequent step, just want to make sure)

@nsoranzo
Copy link
Member

nsoranzo commented Dec 12, 2016

Both make npm-deps and make client work fine for me.

@dannon
Copy link
Member Author

dannon commented Dec 12, 2016

Ok, I reproduced this on a linux box, while my osx config was working fine, let me see what's going on here.

@galaxybot galaxybot added this to the 17.01 milestone Dec 12, 2016
@dannon
Copy link
Member Author

dannon commented Dec 12, 2016

@nsoranzo Try now.

@nsoranzo
Copy link
Member

@dannon

$ make charts
cd client && npm install
npm WARN install Couldn't install optional dependency: Unsupported
client/node_modules/webpack/bin/webpack.js -p --config config/plugins/visualizations/charts/webpack.config.js
module.js:340
    throw err;
    ^

Error: Cannot find module 'grunt'
    at Function.Module._resolveFilename (module.js:338:15)
    at Function.Module._load (module.js:289:25)
    at Module.require (module.js:366:17)
    at require (module.js:385:17)
    at Object.<anonymous> (/usr/users/ga002/soranzon/software/galaxyproject_galaxy/config/plugins/visualizations/charts/webpack.config.js:3:13)
    at Module._compile (module.js:425:26)
    at Object.Module._extensions..js (module.js:432:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:313:12)
    at Module.require (module.js:366:17)
make: *** [charts] Error 1

It's not finding grunt module now.

@nsoranzo
Copy link
Member

No change with a0ae63d

@dannon
Copy link
Member Author

dannon commented Dec 12, 2016

@nsoranzo Got it, this should set the path correctly now. My global node environment happened to have 'grunt', explaining that version working fine for me.

Edit: And, our comments passed in the ether. I'm going to have to revisit this in a bit if it's still not working for you.

Oh, and node --version?

@nsoranzo
Copy link
Member

This fixed it for me:

$ git diff Makefile
diff --git a/Makefile b/Makefile
index c7e6e55..03bebe4 100644
--- a/Makefile
+++ b/Makefile
@@ -13,6 +13,7 @@ IN_VENV=if [ -f $(VENV)/bin/activate ]; then . $(VENV)/bin/activate; fi;
 PROJECT_URL?=https://github.com/galaxyproject/galaxy
 GRUNT_DOCKER_NAME:=galaxy/client-builder:16.01
 GRUNT_EXEC?=node_modules/grunt-cli/bin/grunt
+WEBPACK_EXEC?=node_modules/webpack/bin/webpack.js
 DOCS_DIR=doc
 DOC_SOURCE_DIR=$(DOCS_DIR)/source
 SLIDESHOW_DIR=$(DOC_SOURCE_DIR)/slideshow
@@ -92,6 +93,9 @@ client-install-libs: npm-deps ## Fetch updated client dependencies using bower.
 
 client: grunt style ## Rebuild all client-side artifacts
 
+charts: npm-deps ## Rebuild charts
+       NODE_PATH=client/node_modules client/$(WEBPACK_EXEC) -p --config config/plugins/visualizations/charts/webpack.config.js
+
 grunt-docker-image: ## Build docker image for running grunt
        docker build -t ${GRUNT_DOCKER_NAME} client
 

The issue is that something in the Makefile exports NODE_PATH=MYGALAXYDIR/.venv/lib/node_modules, so ?= was not changing it.

@dannon
Copy link
Member Author

dannon commented Dec 12, 2016

@nsoranzo Interesting, coincidence upon coincidence, trying to guess remote environment configurations is the best. Any idea what it was setting that env var for you, since it's working on both of my machines? My guess is it's something else in your environment, and not the makefile, since that definition is all the way at the top right?

I'll update with a concrete path, anyway.

@nsoranzo
Copy link
Member

NODE_PATH is defined in my virtualenv because I have node installed using:

$ . .venv/bin/activate
(.venv)$ pip install nodeenv
(.venv)$ nodeenv -p

On the other hand, this is loaded by the Makefile with:

IN_VENV=if [ -f $(VENV)/bin/activate ]; then . $(VENV)/bin/activate; fi;

@dannon
Copy link
Member Author

dannon commented Dec 12, 2016

Did not expect nodeenv, got it. Now your setup makes sense to me.

@nsoranzo nsoranzo merged commit c7568a1 into galaxyproject:dev Dec 12, 2016
@nsoranzo
Copy link
Member

Thanks @dannon!

@guerler
Copy link
Contributor

guerler commented Dec 12, 2016

Thanks a lot @dannon.

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

Successfully merging this pull request may close these issues.

4 participants