mgr/dashboard: remove node/npm system installation #20898
Conversation
tspmelo
force-pushed
the
wip-nodejs-improvement
branch
from
1d12aee
to
b1dbe9f
Compare
| message(FATAL_ERROR "WITH_MGR_DASHBOARD_V2_FRONTEND set, but npm not found") | ||
| endif() | ||
|
|
||
| set(mgr-dashboard_v2-nodeenv ${CMAKE_SOURCE_DIR}/build/dashboard_nodeenv) |
There was a problem hiding this comment.
should use CMAKE_BINARY_DIR instead of hardwiring it to ${CMAKE_SOURCE_DIR}/build. also i'd suggest remove the prefix of mgr-dashboard_v2-, and install the nodeenv in current binary, i.e.
set(mgr-dashboard_v2-nodeenv ${CMAKE_CURRENT_BINARY_DIR}/node-env)
| COMMAND ${CMAKE_SOURCE_DIR}/src/tools/setup-virtualenv.sh ${mgr-dashboard_v2-nodeenv} | ||
| COMMAND ${mgr-dashboard_v2-nodeenv}/bin/pip install nodeenv | ||
| COMMAND ${mgr-dashboard_v2-nodeenv}/bin/nodeenv -p -n 8.10.0 | ||
| WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/build |
tspmelo
force-pushed
the
wip-nodejs-improvement
branch
from
b1dbe9f
to
5fdd741
Compare
|
Implemented @tchaikov's suggestions and updated |
|
|
||
| .. note:: | ||
| After you are finished with it, you can simply run ``deactivate`` and exit the |
There was a problem hiding this comment.
this scares me =)
might want to put something like
Once you finish it
tspmelo
force-pushed
the
wip-nodejs-improvement
branch
2 times, most recently
from
5faf2b2
to
e6743f6
Compare
|
updated |
|
It would be nice to skip using nodeenv if running on a distro that has recent enough node/npm (Fedora 27 has it, probably also latest opensuse has it?). I get that it is more complicated to have both the native path and the nodeenv path though. |
|
@tspmelo I tested the frontend build plus frontend unit tests in tumbleweed, ubuntu 16.04, 18.04, and CentOS 7 docker containers and everything worked good! But before merging this code as it is, we could take this opportunity to move the frontend build artifacts from the source tree into the build directory, what do you think? |
Node and npm are now being installed in a virtualenv, removing the need of having it installed in the system or as a node dependency. Now, if you want to use npm, you just need to activate the virtualenv created on 'build/src/pybind/mgr/dashboard/node-env', and then you can execute the same commands as you did before. Signed-off-by: Tiago Melo <tmelo@suse.com>
tspmelo
force-pushed
the
wip-nodejs-improvement
branch
from
e6743f6
to
d5f8713
Compare
|
Rebased the branch. @jcsp are you talking about implementing this on |
LenzGr
changed the title
@tspmelo second thought, moving the location of the frontend files requires changes to the backend code. That should be done in a different PR. Therefore, this PR LGTM |
|
@tspmelo Thanks, this makes the whole make-dist experience much smoother. |
|
@tspmelo builds in shaman were successful |
Node and npm are now being installed in a virtualenv, removing the need of
having it installed in the system or as a node dependency.
Now, if you want to use npm, you just need to activate the virtualenv created
on 'build/src/pybind/mgr/dashboard/node-env', and then you can execute
the same commands as you did before.
Signed-off-by: Tiago Melo <tmelo@suse.com>