[18.05] add static content for vis plugins

[martenson]

is this what is needed to make visualizations work on release branch? It is not even compressed so we shouldn't merge as is :/

ping @guerler

[dannon]

Whoa, did this get stripped or something, previously? This should absolutely not be necessary -- the whole point of the bundling was to make portable, atomic plugins that do not have to be rebuilt all the time.

What was the actual problem you were seeing that prompted this?

[martenson]

@dannon visualizations on Main fail with 404s on Request URL: https://usegalaxy.org/static/plugins/visualizations/nvd3/nvd3_bar/static/nvd3.js

sorry to interrupt your vacation, I was trying to avoid it

[dannon]

@martenson No worries at all, randomly happened to see the email come in when I came up for lunch ;) I'm trying to stay away, but it's hard.

I see what's going on here. The visualization framework requires staging now, with #5810, if you aren't using the predefined routes for static content.

[martenson]

The visualization framework requires staging now

how is this done?

[dannon]

The plugins gulp task does it, which is part of the default build task, with the expectation that this all happens automagically on startup moving forward eventually. The intent was for the old (required) nginx mapping to continue to work, though. My guess is (if this is broken on main) maybe that it seems like we potentially disabled the mapping on main, but aren't actually doing the build to push this stuff, as intended?

In any event, the fix here shouldn't be extra (new) static, if that helps. If anything, we may need to revert some of the routing changes, if those are busted.

[martenson]

I see three options:

• map static to config (cannot be done/hard for uwsgi deployments)
• build client on startup (we agreed we will not require this and make effort to ship pre-built static)
• this PR

After discussions with @guerler and @natefoo I think the preferred choice is actually merging this PR.

edit: this is broken on Main since yesterday, when we turned off the client building, but is broken for everybody else with uwsgi since 18.05

[dannon]

For main, right now, just run 'gulp plugins' wherever static is served. As you mention, turning off the client deployment process is what broke it. That command would fix the staging right now without any commits. If that's broken, we should fix it. yarn run gulp plugins would be the full command.

Longer term, like we've talked about, this should just automagically happen on startup for most sites.

Medium term, instead of completely duplicating 50k lines adding them to static, when we're actively trying to do the opposite and remove these artifacts from the repo, let's make it stage when the route is requested, or when the plugin is loaded. That'd be a further improvement, instead of a step backward?

Edit: Additional options are to

• just run gulp plugins as a part of startup, and not a client build, even on deployed branches. No client changes, just mirrors plugin static to the expected static serving location
• if we're worried about the node environment, just rewrite this plugin staging code (like 10 lines of javascript) in python, and have it happen on startup across the board.

These two options are actually better, since they'll correctly stage/route deployer-installed plugins. Adding all this static to the repo does nothing for that.

Regarding (we agreed we will not require this and make effort to ship pre-built static) -- there's no actual "building" that needs to be done here -- all of the prebuilt artifacts are in the repo -- it's just copying prebuilt artifacts from one location to another to make the static/* routing easier for plugins via gulp task.

I don't think we should merge this.

[natefoo]

when we're actively trying to do the opposite and remove these artifacts from the repo

Is that the goal? My recollection is that we'd agreed that build artifacts would remain for released stable branches. That said, if all that's needed here is a copy, then let's just have them copied in common_startup.sh or whatever.

[jmchilton]

If we are modifying, copying, etc.. the artifacts at startup - why not just build them? The Conda based node stuff seems pretty stable and should work on all older OSs we support otherwise. I propose we merge this PR as is so 18.05 has the correct stable artifacts we sort of promised and immediately branch a 18.06 from 18.05 that just removes the artifacts all together - and all future releases will require a build. We've been waiting too long on pip install galaxy and it doesn't seem to be on the horizon.

[martenson]

@jmchilton what is the purpose of 18.06, why not just wait for next regular release?

[jmchilton]

Because you and @natefoo want a stable release with the artifacts in place and ready, @dannon wants to not include these in the release. I'm compromising and giving you each a release that does what you want. It also gives admins a clean release that only changes that to help upgrade. I want to backport some bugfixes to GUI stuff and I'm tired of dealing with the build artifacts.

[dannon]

Again, I don't think we should merge this. It's a simple stage - a flat copy/duplication of 50k lines, all this bloats the repo.

I only used the gulp process to do it because it was easy to include there -- maybe not a great choice in retrospect, and it'd be trivial as @natefoo suggests to just stage with sh in run, or do it in python.

My plan was, yes, to include build artifacts until we have a robust way of automatically building correctly. We have that now, and I hoped we'd drop them completely in the next release or two.

[dannon]

Can ya'll just run yarn run gulp plugins to get main working, if it hasn't been done already, and sit on this until I get back on Monday? I promise I can come up with a better fix that'll make everyone happy, if nobody else gets to it first.

[jxtx]

Why is doing the mapping so hard?

…

On Wed, Jun 6, 2018 at 4:22 PM Martin Cech ***@***.***> wrote: I see three options: - map static to config (cannot be done/hard for uwsgi deployments) - build client on startup (we agreed we will not require this and make effort to ship pre-built static) - this PR After discussions with @guerler <https://github.com/guerler> and @natefoo <https://github.com/natefoo> I think the preferred choice is actually merging this PR. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#6297 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAE4ZaqvQjLWm0Y989ZwjlDTaQ7LmUC8ks5t6Dn4gaJpZM4UdJUS> .

[dannon]

@jxtx uwsgi static mapping didn't play nice with the dynamic routes required for multi-tiered plugins (and we need the granular mapping to only serve the static content, instead of the python/package/etc files that might be in plugins)

[jxtx]

Is there an issue for that? Seems like that is what should be fixed as we move to uwsgi by default.

…

On Thu, Jun 7, 2018 at 9:58 AM Dannon ***@***.***> wrote: @jxtx <https://github.com/jxtx> uwsgi didn't play nice with the dynamic routes required for multi-tiered plugins — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6297 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAE4ZZoGPk76L00LuA7J442HHnh_GHUPks5t6TGPgaJpZM4UdJUS> .

[dannon]

@jxtx Yeah, #5738 talks more about the problem. #5810 was the fix we decided on.

[jmchilton]

a flat copy/duplication of 50k lines, all this bloats the repo.

My understanding is this level of one time bloat is tiny compared the packed artifacts we are regenerating with every bug fix to 18.05. If we merge this and then create an 18.06 and push client fixes to that - the growth size of our repo will slow way down.

[dannon]

@jmchilton My simple patch check says this is 9.2 Mb. So that's 18 megs to do/undo it, which is significant.

(to be clear, I am 100% with you on dropping them from the repo completely -- I just don't think this particular PR or 'fix' should be pursued at all)

[jmchilton]

I'd assume the undo wouldn't count twice - but 9Mb is significant - I'm on your side I guess. We should just require extra intervention (using whatever mechanism we want in run.sh) to produce the release artifacts in 18.05 then despite our intention was originally to avoid that. I'm still on the 18.06 train though because I do care about the size of the repo and because if we are requiring intervention to generate the static content we should just generate it all by default.

Update: Wait is the patch 9Mb or is the size it contributes to the git repo 9Mb? How could this possibly be worse than the packed everything once in the reoo?

[martenson]

let's just have them copied in common_startup.sh or whatever.

+1

immediately branch a 18.06 from 18.05 that just removes the artifacts all together

+1 on doing this but after GCC (July/August) to prevent user/admin/training confusion and give us more time to think about this.

[dannon]

@jmchilton That was the patch size, I didn't check the git index to see the blob, not sure.

@martenson Thanks for that, and I agree with it being post-GCC. I pinged @natefoo on IRC and we're going to get main staged right away for an immediate fix, and I can look into run.sh when I get back.

If anyone is interested in looking before that, here's the handle to the gulp task that does the staging (all that we need to do to make everything work): https://github.com/galaxyproject/galaxy/blob/dev/client/gulpfile.js#L115

[martenson]

Main fixed now, the release needs a bugfix that would copy the config static to static proper.

p.s. we could also symlink-mirror it, haha

[martenson]

on 18.05

$ ./node_modules/.bin/gulp plugins

gulp[22607]: ../src/node_contextify.cc:629:static void node::contextify::ContextifyScript::New(const FunctionCallbackInfo<v8::Value> &): Assertion `args[1]->IsString()' failed.
 1: node::Abort() [/usr/local/bin/node]
 2: node::MakeCallback(v8::Isolate*, v8::Local<v8::Object>, char const*, int, v8::Local<v8::Value>*, node::async_context) [/usr/local/bin/node]
 3: node::contextify::ContextifyScript::New(v8::FunctionCallbackInfo<v8::Value> const&) [/usr/local/bin/node]
 4: v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo*) [/usr/local/bin/node]
 5: v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<true>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/usr/local/bin/node]
 6: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/usr/local/bin/node]
 7: 0x275635b8427d
 8: 0x275635b8f755
 9: 0x275635c0bc20
10: 0x275635b944f7
11: 0x275635b944f7
12: 0x275635b944f7
Abort trap: 6

[martenson]

new node 10.4 broke this, stupid yarn still supports only 8.x