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

Build endpoint separation #5058

Merged
merged 11 commits into from Nov 30, 2017

Conversation

Projects
None yet
5 participants
@dannon
Member

dannon commented Nov 22, 2017

This more formally separates development and production client builds. The default make client is now a developer-focused build which generates artifacts that should never be committed. make client-production is the only thing that should ever be committed to the repo moving forward, only once per PR by either mergebot or the individual responsible for the merge (prior to mergebot being available).

Frontend developers will note non-obfuscated variable names (the equivalent of GXY_NOPACK previously), sourcemaps for bundles, etc.

The bottom line here is more developer-friendly dev builds, and faster, more optimized production builds.

Includes removal of a couple extra .map files that seemed to cling to the repo after the previous purge.

dannon added some commits Nov 22, 2017

Slight refactoring in package.json to standardize with 'dev' being th…
…e common, default option and 'production' being the non-default (logic being a build system should be the thing remembering extra options over the default)

@dannon dannon added the status/WIP label Nov 22, 2017

@dannon dannon referenced this pull request Nov 22, 2017

Merged

Sourcemap removal #5054

@martenson

This comment has been minimized.

Member

martenson commented Nov 22, 2017

Sourcemaps are very useful for production.

@guerler

This comment has been minimized.

Contributor

guerler commented Nov 22, 2017

@martenson that is a good point, they can be helpful with identifying bugs on main and other servers in production or am I misunderstanding 'very useful'?

@galaxybot galaxybot added this to the 18.01 milestone Nov 22, 2017

@martenson

This comment has been minimized.

Member

martenson commented Nov 25, 2017

@guerler correct, that is the main usecase I see for them

@martenson

This comment has been minimized.

Member

martenson commented Nov 29, 2017

I am not sold on what is presented as facts here - that we need to branch our build process into two and that we need to abandon maps in production envs.

From webpack docs:

We encourage you to have source maps enabled in production, as they are useful for debugging as well as running benchmark tests.

Would you like to comment on this issue @dannon ?

@dannon

This comment has been minimized.

Member

dannon commented Nov 29, 2017

@martenson This PR does not change a single thing relative to current -dev about the production build regarding sourcemaps. It's quality of life changes for local development. (which "presented as facts" are you talking about?)

The compromise that I talked about with @guerler was to have a separate, explicit build target for people who want to host and use sourcemaps (since with their removal from the repo, it requires a build anyway).

Edit: The PR now does, due to subsequent commits, change sourcemaps relative dev -- that is, in supporting them.

@dannon

This comment has been minimized.

Member

dannon commented Nov 29, 2017

Ok, should be good to go, supporting mapped and unmapped 'production' style builds @guerler

dannon added some commits Nov 29, 2017

@jmchilton

This comment has been minimized.

Member

jmchilton commented Nov 29, 2017

  • make client should generate source maps - they are useful for local development - I had to re-enable them locally for testing my client PRs today.
  • make client-production should default to producing source maps that are excluded from the repository. source maps are useful for production servers - we just don't want to include them in the server. I don't believe anyone has presented a tangible downside to generating and committing artifacts that reference missing sources maps that out weighs the reproducibility concerns of having to generate new, non-shipped client bundles just to have source maps included in production.

I would expect that most production instances will want to generate source maps, that we will generate them for the Docker images, in the ansible recipes, etc.... We should generate bundles compatible with that.

Updated: To be clear though - if speed of source map generation is a concern - I have no problem having a separate make target for disabling it - I just think for both development and production the default of generating source maps makes more sense.

@dannon

This comment has been minimized.

Member

dannon commented Nov 29, 2017

@jmchilton Did you actually run make client? It does generate sourcemaps.

I do not follow 'generating bundles compatible with that'. The bundles aren't incompatible with sourcemaps. They're accurate. If a deployer wants to ship sourcemaps, since we don't ship them in the repository, they will by necessity run one of the various build commands. These are going to rebuild bundles, so it doesn't even matter what's in the repository. We may as well put accurate contents in the repository, no? Docker images, ansible recipes, these will all build their own bundles and maps, using the command provided. The maps aren't in the repo. I think it is much more clear to have an explicit endpoint for generating these, rather than having repo artifacts reference things that aren't even there.

This is the most inane, inconsequential thing we have ever had a week-spanning waste-of-time argument on. Do we really have nothing better to do? If this is a -1 for you, whether we build inaccurate artifacts to put in the repository, then I yield. This is not worth the time we've I've already spent on it.

@jmchilton

This comment has been minimized.

Member

jmchilton commented Nov 29, 2017

Did you actually run 'make client'? It does generate sourcemaps.

On this PR or in dev - I just restested the latest dev and it doesn't unless I'm doing something crazy. My reading of this PR was the make client wouldn't generate source maps. If it does - then absolutely fantastic.

As for the rest of it - we have reproducible bundle generation I think - and that is fantastic. If we ship bundles that have gone through Selenium tests - than as a deployer I'd want to use those I think. Generating source maps compatible with them is like gravy. I think it would be reassuring to see the git diff is that the bundles haven't changed but now I have source maps. It is a matter of balancing a sort of esoteric concern about referencing missing files with a sort of esoteric concern about reproducibility and degree of testing.

As for the -1 comment - I'm not -1 on this - I told @guerler out of band if he wants to merge it is as is he should totally feel free to - it is a big step forward from the current dev. I just wanted to express my concerns that the old status quo was better and that I think we are making some incorrect tradeoffs IMO.

@dannon

This comment has been minimized.

Member

dannon commented Nov 29, 2017

As a deployer, you're going to overwrite any bundles that exist in the source tree when you run make <blah>. Are you really just talking about the ease of seeing the difference when manually inspecting the diffs?

@jmchilton

This comment has been minimized.

Member

jmchilton commented Nov 29, 2017

As a deployer, you're going to overwrite any bundles that exist when you run 'make '.

Are the commands no longer reproducible? Before if you would run make client over and over you'd get the same result. Is that not the case anymore? If it is still the case - I'd argue that overwriting a file with the same contents doesn't affect reproducibility or the degree to which the artifacts have been tested.

@dannon

This comment has been minimized.

Member

dannon commented Nov 29, 2017

The commands should build the same artifacts, but varying node environments can be different, and they have been in the past. Even if they're the same, my point is that they're being overwritten, and literally the only thing we're talking about is visual inspection of the git status/diff. Deployments should be automated in any reasonable situation, and this should be a rare occasion/concern. If you need to check an environment you can easily make production, make sure it's sane (i.e. matches your git status expectations), and then make production-maps. If it'll allay any fears, I'll write a script to verify the sourcemap built artifacts are legit with respect to the committed source and include it in the repo. The bottom line, for me, is that this approach is accurate with respect to what is in the repo, and is explicit in stating that there are no source maps shipped. I think this is the correct way to do things, and it outweighs any perceived negatives.

(my guess is that most people are not going to bother with npm/yarn/whatever to build sourcemaps for deployment -- it's the rare case here)

@martenson

This comment has been minimized.

Member

martenson commented Nov 30, 2017

After this gets merged what client build (if any) will we have on release_ and dev branches?

@dannon

This comment has been minimized.

Member

dannon commented Nov 30, 2017

@martenson make client-production is what will be committed, on any branch. We'll do this manually (just like we do now) until the mergebot (#5055) is available to automatically handle this on merge, after which there shouldn't be a need for anyone to ever build (except to develop and test locally, of course) and commit anything in /static.

@martenson

This comment has been minimized.

Member

martenson commented Nov 30, 2017

client-production-maps is what I would like to see on release_ branches.

@dannon

This comment has been minimized.

Member

dannon commented Nov 30, 2017

@martenson That makes me think of a good option -- for now, make client-production for commits to dev, and we have a larger group discussion about this with the next release meeting to decide what that the release practice should be?

It's apparent there are two differing viewpoints that don't seem a whole lot closer to being reconciled, and given that, we should seek more data (on what deployers will actually want, and do) and more opinions to make any progress on what the release practice should be.

The best news is that this PR doesn't mandate one way or another, it's just tooling, and we can hopefully close it out merge it (on reread, it seemed like I should make that clearer) and move on, revisiting the packaging issue at release.

@guerler guerler merged commit 5e17e44 into galaxyproject:dev Nov 30, 2017

7 checks passed

api test Build finished. 317 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 163 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 58 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript No alert changes
Details
selenium test Build finished. 100 tests run, 1 skipped, 0 failed.
Details
toolshed test Build finished. 577 tests run, 0 skipped, 0 failed.
Details
@dannon

This comment has been minimized.

Member

dannon commented Nov 30, 2017

Thanks @guerler.

@guerler

This comment has been minimized.

Contributor

guerler commented Nov 30, 2017

I merged this since it will allow us to build the client with and without source maps. We can separately discuss which of the two should be the default mode. Thanks a lot for working on this @dannon.

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