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

[deploy.sh] fix node-gyp failure on rebuild #191

Merged
merged 2 commits into from
Oct 5, 2020

Conversation

moriarty
Copy link
Contributor

node-gyp configure will fail if build directory already exists.

One reason this might already exists is if new models have been added
to the model path, and npm run deploy --- -m local is run again.

This change enables building a generic docker image for gzweb, and later
loading in the models at launch time via a volume mount that were not
known when the original gzweb image was built.

@moriarty
Copy link
Contributor Author

@iche033 & @chapulina I must admit I'm no expert in nodejs so this might be a hack.

There are some deprecated and old tags for gzweb here:
https://hub.docker.com/r/osrf/gazebo/tags

And there are no images or tags of gzweb in the new official location here:
https://hub.docker.com/_/gazebo/tags

The images here:
https://github.com/osrf/docker_images
are out of date https://github.com/osrf/docker_images/blob/master/gazebo/9/ubuntu/bionic/gzweb9/Dockerfile and haven't been updated since the bitbucket transition osrf/gazebo_tutorials#118 but they also needed a few more changes.

In general, the old gzweb docker_image in https://github.com/osrf/docker_images seems more like an sample of how to put gzweb into docker, it starts a gzserver (without many plugins available or the mechanisms to load custom ones in place) and gzweb which is probably not what it would do in. @mikaelarguedas might have some insight here as I've opened issues to docker_images in the past and to use them in practice usually you end up writing your own.

In practice, I believe it's possible to create a generic gzweb docker image, which is not aware of all the required models ahead of time, but when brought up with kubernetes or docker-compose, unknown models could be volume mounted to the gzweb container, and added to the GAZEBO_MODEL_PATH environment variable, and the command run by gzweb would first re-run deploy -m and then run start...

Not actual code, just an example of how this change would enable updating the official gzweb images for generic use:

version: "3"
  gz-server:
    ...
    volumes:
      - gz-server-models:/path/to/the/actual/models

  gz-web:
     image: could.be.official.registry/gzweb:latest
     depends_on:
       - gz-server
     environment:
       - GAZEBO_MASTER_URI=gz-server:11345
       - GAZEBO_MODEL_PATH=/path/to/mount/models
     volumes:
       - gz-server-models:/path/to/mount/models:ro
     command: >
       bash -c "npm run deploy --- -m local
       && npm start"

volumes:
  gz-server-models:

@moriarty
Copy link
Contributor Author

moriarty commented Sep 1, 2020

GitHub actions were not triggered on this PR see PR #192

node-gyp configure will fail if build directory already exists.

One reason this might already exists is if new models have been added
to the model path, and `npm run deploy --- -m local` is run again.

This change enables building a generic docker image for gzweb, and later
loading in the models at launch time via a volume mount that were not
known when the original gzweb image was built.
From nodejs/node-gyp README:
- `rebuild`  Runs `clean`, `configure` and `build` all in a row
- `clean`    Removes the `build` directory if it exists

This ensures `npm run deploy` succeeds if it is run a second time.
`npm run deploy` needs to be re-run if the deploy options change.
@moriarty moriarty force-pushed the moriarty/node-gyp-fails-rebuild branch from 962b8d7 to 9a649a0 Compare September 10, 2020 16:53
@moriarty
Copy link
Contributor Author

Ignore all the docker reasoning I mentioned... I think this is simply a bug.
https://github.com/nodejs/node-gyp/blame/9bf112b44c98be39bf8770a1341fa4ddf23556d4/README.md#L167-170

@moriarty
Copy link
Contributor Author

moriarty commented Oct 5, 2020

Bump @iche033 and @chapulina
I'll copy from the nodejs/node-gyp readme link I included above her for quicker reference

Command Description
help Shows the help dialog
build Invokes make/msbuild.exe and builds the native addon
clean Removes the build directory if it exists
configure Generates project build files for the current platform
rebuild Runs clean, configure and build all in a row
install Installs Node.js header files for the given version
list Lists the currently installed Node.js header versions
remove Removes the Node.js header files for the given version

This PR replaces:

  • configure
  • build

with just:

  • rebuild

Because build will fail when the build directory isn't clean.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me

@chapulina chapulina merged commit 736a625 into osrf:master Oct 5, 2020
FelipeGdM pushed a commit to FelipeGdM/gzweb that referenced this pull request Feb 1, 2021
* [deploy.sh] fix node-gyp failure on rebuild

node-gyp configure will fail if build directory already exists.

One reason this might already exists is if new models have been added
to the model path, and `npm run deploy --- -m local` is run again.

This change enables building a generic docker image for gzweb, and later
loading in the models at launch time via a volume mount that were not
known when the original gzweb image was built.

* [deploy.sh] node-gyp rebuild

From nodejs/node-gyp README:
- `rebuild`  Runs `clean`, `configure` and `build` all in a row
- `clean`    Removes the `build` directory if it exists

This ensures `npm run deploy` succeeds if it is run a second time.
`npm run deploy` needs to be re-run if the deploy options change.
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.

2 participants