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

Client build check #5128

Merged
merged 4 commits into from Jan 3, 2018

Conversation

Projects
None yet
7 participants
@dannon
Member

dannon commented Dec 5, 2017

Ok, so the idea here is that we'll drop prebuilt artifacts from dev. To do that, we need a way to communicate to folks when their client build is missing or out of sync, which will of course cause Galaxy to fail to work.

This simple check augments run.sh to check if someone is running dev, looks for a specified client-build hash file that I've added (this is .gitignored), and compares the build hash to the current git hash. If the client-build hash file doesn't exist, or if the hash is out of date, run.sh exits with a message saying "Hey, you need to build the client". Building the client through any of the three 'full build' targets adds this hash file.

Other branches will skip this check entirely, since it only applies to dev (for now). Release branches will include correct artifacts (again, for now). I could swap this from branch == dev to branch != release_*, if we're worried people will be on non-dev non-release local branches that should include this client handling.

xref #5055 (some related conversation here about build artifacts)

@galaxybot galaxybot added this to the 18.01 milestone Dec 7, 2017

@bgruening

This comment has been minimized.

Member

bgruening commented Dec 8, 2017

I guess the big problem with this is that the developer needs to have yarn installed, even if he is not familiar with the client side.
Could we get this automatically installed with conda?

  • Postponing the client side check after the miniconda env is setup
  • installing yarn/node into miniconda
  • generate the client / give the user a command with conda activate and yarn
  • make sure conda is not on PATH anymore

Or is this to much magic? Obviously we could do this only if yarn is not installed.

@dannon

This comment has been minimized.

Member

dannon commented Dec 8, 2017

@bgruening Personally as a developer (speaking about any project here, not just Galaxy) I'd rather see a notification and a set of options for installing required dev tools over automagic bootstrapping I might not then understand.

If there's a conda-based shorthand we could certainly provide that in the client/README.md. Another option is adding a docker-based build, and mentioning that. make docker-client-production or whatever, which just uses yarn in docker to do all the same stuff.

My thinking was that, at least for developers, "See client/README.md for more information building this software" is the more common occurrence over automatically installing stuff.

@bgruening

This comment has been minimized.

Member

bgruening commented Dec 8, 2017

@bgruening Personally as a developer (speaking about any project here, not just Galaxy) I'd rather see a notification and a set of options for installing required dev tools over automagic bootstrapping I might not then understand.

I can understand this and share the opinion, but we also have a lot backend-devs only that don't want or are not familiar with the JS stuff. Not sure how much this is a push back for them.

If there's a conda-based shorthand we could certainly provide that in the client/README.md. Another option is adding a docker-based build, and mentioning that. make docker-client-production or whatever, which just uses yarn in docker to do all the same stuff.

Sure, this can also work nicely if you have Docker.

My thinking was that, at least for developers, "See client/README.md for more information building this software" is the more common occurrence over automatically installing stuff.

Yes, in the end its about communication. But I think we should provide a very easy way to get the client side build machinery for people that are not familiar with yarn et al.

@dannon

This comment has been minimized.

Member

dannon commented Dec 8, 2017

@bgruening Another option is that I can probably make the client build work with npm or yarn interchangeably. Yarn should be the authoritative source, since we use that version locking (so, 'official' client builds will always be yarn'd), but either one might work for a quick local developer deployment?

@bgruening

This comment has been minimized.

Member

bgruening commented Dec 19, 2017

with npm or yarn interchangeably

I don't think this is necessary. yarn and npm are probably equally complicated to install. Lets just try if conda can be used to get yarn and then build the client - I fear that an activated conda will mess around with the venv.

@dannon

This comment has been minimized.

Member

dannon commented Dec 20, 2017

@bgruening Ok, I won't try to make it work both ways (npm and yarn). Less work for me :)

How about I just add a little bit of extra help text, and a better pointer of where to go for help, and we try this out? I'm eager to stop committing build artifacts to dev, completely.

@mvdbeek

This comment has been minimized.

Member

mvdbeek commented Jan 3, 2018

Looks good to me, I don't think we should defer to Conda for yarn.
People should use whatever package manager they like for installing yarn, I think this is up-to-date everywhere and https://yarnpkg.com/en/docs/install is helpful.

@dannon

This comment has been minimized.

Member

dannon commented Jan 3, 2018

@mvdbeek Totally agreed, thanks for taking a look at this.

edit, afterthought: I do point people to yarnpkg.org in the client readme -- do you think we need to point people directly to /docs/install, or is the pointer that is there sufficient?

@bgruening

This comment has been minimized.

Member

bgruening commented Jan 3, 2018

I think the discussion was how we can get Galaxy to build the client without installing yarn manually. So to keep the status quo if we don't ship client builds. People that want to hack on the client are probably capable of installing yarn from wherever. But what do we do about people that just want to run dev, hack the backend etc and don't know about yarn etc...

@martenson

This comment has been minimized.

Member

martenson commented Jan 3, 2018

To me this is similar to us providing conda for tool deps, I really like the seamless 'now you have conda configured and you didn't move a finger' experience. It would be great if we can deliver this to any dev checkouter.

@dannon

This comment has been minimized.

Member

dannon commented Jan 3, 2018

@bgruening I think I'd prefer handling this by, instead of extra automation, increased developer outreach and education where necessary. Not sure if you saw, but I updated the help text with a lot more information on how to install, and how to contact us if you have any trouble, and I feel pretty comfortable that it's all a very easy process with lots of ways to get help built in. If a developer really only wants to touch the back-end, and perhaps verify their changes via the API or tests, etc., there's always the '--no-client-build' option that is also mentioned in the error message.

I'd really like to move forward with this as-is, without extra installation automation and solve those extra problems if and when we need to, instead of getting ahead of ourselves with automation that we have to then manage. Personally, I know I don't want any application installing yarn, automake, etc., for me. I'd rather do that myself.

To me, this isn't like conda for tool deps, it's like a package expecting me, a developer, to have a c compiler, automake, etc., to build their package. Except it's better because it actually allows me to skip it if I don't want to ;)

@mvdbeek

This comment has been minimized.

Member

mvdbeek commented Jan 3, 2018

To find a middle ground, how about we print out sth. like

Could not find yarn!
To install yarn, visit https://yarnpkg.com/en/docs/install, or run the following script
that will install yarn for you.

curl -o- -L https://yarnpkg.com/install.sh | bash

when running make client and yarn is not on PATH.

I really wouldn't encourage satisfying system galaxy dev dependencies with conda,
too many things can go wrong there.

In any case I don't think the question if/how yarn will be provided should stop this PR,
since the PR only checks if a client build is necessary and provides a mechanism to ignore the warning.

@dannon

This comment has been minimized.

Member

dannon commented Jan 3, 2018

@mvdbeek Having a yarn check when running the node-deps target in the Makefile sounds good to me, I can add that either here, or, in a separate PR.

@martenson martenson merged commit bb29444 into galaxyproject:dev Jan 3, 2018

6 checks passed

api test Build finished. 336 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 165 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 60 tests run, 0 skipped, 0 failed.
Details
selenium test Build finished. 118 tests run, 2 skipped, 0 failed.
Details
toolshed test Build finished. 577 tests run, 0 skipped, 0 failed.
Details
@martenson

This comment has been minimized.

Member

martenson commented Jan 3, 2018

In any case I don't think the question if/how yarn will be provided should stop this PR,
since the PR only checks if a client build is necessary and provides a mechanism to ignore the warning.

Good point, I did not want to derail this. Sorry.

@dannon

This comment has been minimized.

Member

dannon commented Jan 3, 2018

Thanks folks. I'm excited about the progress our build infra has made, and about not ever committing build artifacts to -dev ever again :)

I'll open another PR with more help text in the node-deps target shortly.

@natefoo

This comment has been minimized.

Member

natefoo commented Jan 16, 2018

Jumping in late here but we've put a lot of effort in over the years to making git clone .. && run.sh just work, and I think we can probably continue to do that here. Is there a reason that, if yarn does not exist on $PATH, we can't run the curl-pipe-bash installer (which hopefully has some options to control where it installs, it appears to go to ~/.yarn by default but ideally it'd be in /path/to/galaxy/.yarn`)?

For people who are more opinionated, we can provide more traditional instructions, e.g.:

  1. Install yarn and pipenv
  2. Create an env, install deps into it
  3. make client
  4. cp galaxy.yml.sample galaxy.yml
  5. uwsgi --yaml galaxy.yml

I fully support removing the built artifacts from dev, but would like to make this as painless as possible for people who don't work on the client (e.g. me).

@dannon

This comment has been minimized.

Member

dannon commented Jan 16, 2018

@natefoo If we really wanted, I guess we could customize https://github.com/yarnpkg/website/blob/master/install.sh, (which is the script hosted for use via curl -o- -L https://yarnpkg.com/install.sh)? This makes me uneasy, though.

I worked on a docker container that'd do the building with preinstalled node_modules a little bit last week, which is what I'd prefer we rely on if we really did want this automatic building on dev. I primarily worked on the container so we could skip all the yarn package installation on CI boxes, but it could easily serve a dual use. Think that'd be enough?

I guess I'm just thinking that for the target audience of dev the instructions aren't a big hoop to jump through, and they educate folks as to what's actually going on, which is a benefit. And, of course all releases are still just git clone .. && run.

@natefoo

This comment has been minimized.

Member

natefoo commented Jan 16, 2018

I think docker as the requirement for a fast dev clone-and-run is probably not ideal if there's an alternative. Turns out the installer is pretty unnecessary, you can just fetch the tarball and untar it to anywhere.

The next problem is that it's written in Node.js - for that we could fetch the appropriate binary tarball from https://nodejs.org/en/download/

@martenson

This comment has been minimized.

Member

martenson commented Jan 16, 2018

@natefoo I think @dannon's approach was to have the container build the client on merge and push it to dev automatically.

@natefoo

This comment has been minimized.

Member

natefoo commented Jan 16, 2018

Oh - but that would put the build artifacts back in the repo, wouldn't it?

@dannon

This comment has been minimized.

Member

dannon commented Jan 16, 2018

@martenson Well, that was back when we thought we still wanted artifacts in dev. We're beyond that now, and we want them gone, which is a good thing.

Since they're gone, we have to have a way to build them locally. Just like we assume someone has a functional python environment, I don't think the new js requirements are a particularly big ask for the dev target audience.

@natefoo I didn't mean docker as the way, but as an available option for building.

@martenson

This comment has been minimized.

Member

martenson commented Jan 16, 2018

@dannon I see, in that case I agree with @natefoo that docker is pretty strong (local) requirement.

If we can really make this as smooth as automatic conda initialization (and @natefoo suggests we can) I am all for it.

@natefoo

This comment has been minimized.

Member

natefoo commented Jan 16, 2018

The difference I think is that Python is almost surely installed, Node.js is typically not. I think we can get the components we need installed pretty easily, I am not sure what the downside is, other than "behind the scenes magic", but we've always had that. I do think we should document the magic, though, as I mentioned above, so people aren't forced to use it.

@dannon

This comment has been minimized.

Member

dannon commented Jan 16, 2018

@natefoo You're welcome to take a stab at it, but for one branch targeted at developers I don't think it's worth the extra moving parts (and I think @mvdbeek suggested the same, above). My plan was to wait and see if anyone actually has a problem with this before trying to fix it.

@mvdbeek

This comment has been minimized.

Member

mvdbeek commented Jan 16, 2018

Contrary to my concerns on gitter using Conda for this was actually pretty easy, as both node and yarn can be installed form conda-forge. I ended up doing this for the one VM at the admin workshop that had a hung background apt process, and it went smooth, so that would be another option.

@dannon

This comment has been minimized.

Member

dannon commented Jan 16, 2018

@mvdbeek Hrmm. That's good to know.

I still think we do developers a disservice 'hiding' the client build process from them, this is something we've struggled with for years and I think it's a real opportunity to encourage folks to learn. Maybe instead of just doing it 'automagically', we throw up a prompt with options?

@natefoo

This comment has been minimized.

Member

natefoo commented Jan 16, 2018

@dannon: I can't speak for everyone but I'm cloning Galaxy on random systems for random minor testing of things I'm developing pretty frequently, and having to manually set up dependencies on those would be a pain. I know I am not representative of your average developer but I really think we have a fairly simple solution here.

@mvdbeek: This seems pretty reasonable to me considering we're autoinitializing conda anyway. We could just create a special _galaxy_client_build env or something?

@natefoo

This comment has been minimized.

Member

natefoo commented Jan 16, 2018

@dannon: We hide the installation of all of our dependencies and the fact that they're pre-built by us, we hide virtualenv creation, we hide conda installation, I am not sure why this is a big departure from how we already work.

@dannon

This comment has been minimized.

Member

dannon commented Jan 16, 2018

@natefoo That makes sense, but can we have it a run option, off by default? So, with --conda-install-client-deps or something pithier (hopefully) it uses conda to install and build automagically, but by default it continues to inform the user and offer options?

@natefoo

This comment has been minimized.

Member

natefoo commented Jan 16, 2018

There are options in run.sh to avoid autocreation of the virtualenv and for skipping dependency management, etc., so we can certainly have the same options for the client.

I am not sure that we want to add a prompt to a script that's supposed to be automateable, and I'd prefer to have it enabled, but I think we also ought to have a discussion (in a new issue?) about how to improve the setup and run procedure anyway. I've never liked run.sh.

@bgruening

This comment has been minimized.

Member

bgruening commented Jan 16, 2018

As stated above I think conda will do the trick. One additional problem is that dev is the default branch, so people will get this branch by cloning galaxy - therefore I would even enable it by default but print a message explaining what is happening, waiting for 5 secs and than installing the conda env.

@dannon

This comment has been minimized.

Member

dannon commented Jan 16, 2018

I think all the hiding and automatic stuff is nice for users, but it's counterproductive (now, and always has been) for a dev branch where it's useful to know how things work.

@natefoo

This comment has been minimized.

Member

natefoo commented Jan 16, 2018

@dannon Ah, I get what you're saying - I agree with that principle quite a bit. I am just not sure if we should draw the line in the sand here, and end up being half-automated and half-not. If we don't want to automate by default then let's not automate anything, but provide a bootstrap script. Otherwise, let's automate everything by default.

@natefoo

This comment has been minimized.

Member

natefoo commented Jan 16, 2018

FWIW we used to have (many, many ages ago) a setup.sh and run.sh but I basically folded setup.sh into run.sh. These could easily be split apart again, as long as people were instructed to run the setup/bootstrap script whenever updating.

@dannon

This comment has been minimized.

Member

dannon commented Jan 16, 2018

I really like the idea of a separate bootstrap script, though I know that's more work than this conversation started out being about :) We could keep the same auto-upgrade behavior by doing something similar in writing a hash of when the bootstrap was run, to know when to tell a person they might need to update their dependencies.

This seems like it's primarily a consequence of us using source control as a distribution mechanism, and having that mechanism be shared by differently aligned audiences with sometimes differing goals. It'd be really great to have a way to, maybe just for the dev branch, pull the curtains up and expose all the magic.

@nsoranzo

This comment has been minimized.

Member

nsoranzo commented Jan 17, 2018

Another option could be to install npm and yarn in the Galaxy virtualenv:

$ . .venv/bin/activate
(.venv)$ pip install nodeenv
(.venv)$ nodeenv -p
(.venv)$ npm install -g yarn
(.venv)$ make client
@mvdbeek

This comment has been minimized.

Member

mvdbeek commented Jan 17, 2018

I like @nsoranzo's suggestion the most, in fact we could consider this to be dev-requirements and handle it like we handle python's dev requirements.

@dannon

This comment has been minimized.

Member

dannon commented Jan 17, 2018

Definitely seems like the easiest way to do it, I like that idea. I didn't know nodeenv was a thing, nifty; I've always used nvm locally.

@natefoo

This comment has been minimized.

Member

natefoo commented Jan 18, 2018

nodeenv looks brilliant for our purposes since we can even use it to ensure a compatible version of node is available. As it is I can't build the client on my workstation because Debian stretch's node is too old, and this is likely the case on many other Linux distributions in common use at the moment.

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