Vmdb::Plugins::AssetPath - add node_modules, development_gem? #17818

himdel · 2018-08-08T13:56:04Z

AssetPath now knows about the path and name of each engine.

But to move node_modules outside of gem directories, we also need to know whether the engine is a gem or not, and need to compute the path to node_modules accordingly.

This (together with ManageIQ/manageiq-ui-classic#4435) will enable the ui tasks to use a different node_modules path for gems, no longer polluting the gem dir.

Cc @Fryguy

@miq-bot add_label wip

There are currently two TODO items on this side:

~~is_gem - I'm not quite sure matching the pathname against bundler/gems is the right way of telling~~
~~/tmp/test/@namespace/node_modules may not be the most universal place for these~~

Fryguy · 2018-08-08T15:45:08Z

lib/vmdb/plugins/asset_path.rb

@@ -7,6 +7,8 @@ class AssetPath
      attr_reader :name
      attr_reader :path
      attr_reader :namespace
+      attr_reader :node_modules
+      attr_reader :is_gem


Prefer gem? over is_gem

That means I need to write a method, but sure :)

Fryguy · 2018-08-08T15:47:19Z

I don't understand the full design of this approach ... why does knowing something is a gem or not matter? technically, they are all gems, just different sources (git vs path vs actual rubygem which we don't have yet) Can you elaborate?

himdel · 2018-08-08T15:50:24Z

Re is_gem.. this seems to work:

          source = bundler_specs_by_path[engine.root.to_s].source
          is_path = Bundler::Source::Path === source
          is_git = Bundler::Source::Git === source
          is_gem = !is_path || is_git

himdel · 2018-08-08T15:55:38Z

Can you elaborate?

Ok, as far as I know, these are the real constraints:

people who never touch the ui code base and never override_gem to a local copy of the ui should always get the "node_modules" outside the directory behaviour - that way, they don't have to rebuild everything every time the UI gem gets updated
people who do touch the ui code base need "node_modules" in the ui-classic dir for yarn and any scripts to work

But.. the same goes for any ui plugin:

if you're not doing any v2v development, you're likely using something like /home/himdel/.rbenv/versions/2.4.3/lib/ruby/gems/2.4.0/bundler/gems/miq_v2v_ui_plugin-7b60bd492758 and don't want node_modules there

if you are doing v2v development, the paths probably looks like ~/miq_v2v_ui_plugin/ and you do want node_modules there.

So.. this is my attempt to detect that. With this...

$ be rake update:print_engines

JS plugins:
  ManageIQ::GraphQL::Engine:
    namespace: manageiq-graphql
    path: /home/himdel/.rbenv/versions/2.4.3/lib/ruby/gems/2.4.0/bundler/gems/manageiq-graphql-5f68621f2791
    node_modules: /tmp/test/manageiq-graphql/node_modules
    is_gem: true
  ManageIQ::UI::Classic::Engine:
    namespace: manageiq-ui-classic
    path: /home/himdel/manageiq-ui-classic
    node_modules: /home/himdel/manageiq-ui-classic/node_modules
    is_gem: false
  ManageIQ::V2V::Engine:
    namespace: manageiq-v2v
    path: /home/himdel/.rbenv/versions/2.4.3/lib/ruby/gems/2.4.0/bundler/gems/miq_v2v_ui_plugin-7b60bd492758
    node_modules: /tmp/test/manageiq-v2v/node_modules
    is_gem: true

himdel · 2018-08-08T15:57:16Z

I don't think we can test for override_gem because you can use override_gem to just pick a different branch from somebody else's repo - in that case I think it's clear that you won't be running any scripts.

OTOH if you check out that repo manually, you probably intend to be running things there.

himdel · 2018-08-08T16:21:33Z

Updated to use development_gem? instead of ! is_gem, and to parse the bundler classes correctly (assuming my assumptions will hold that is :)).

himdel · 2018-08-08T16:39:47Z

Reasons we can't do this for what we now call development gems:

yarn --module-path... will not install any binaries, just packages - yarn run anything won't work - this is the only universal way to run any JS binaries, so.. a problem
running any local JS scripts will not work without NODE_PATH set (of course, nothing else will really work with NODE_PATH set as this is really a per-repo setting)
anything that reads any config from the ui dir will not work without copying everything to the /tmp/test dir (because pretty much everybody essentially does while(!found) { cd .. })

So.. either this only needs to happen for gems where people won't be doing local changes,
or we need all the asset compiling infrastructure to move to the core,
or we can not support running ui-classic as a (non-development) gem for anyone wanting to build assets.
(But only the first really addresses ui plugins, which sucks because plugins are exactly the place where people from other teams would contribute.)

Fryguy · 2018-08-08T19:01:18Z

lib/vmdb/plugins.rb

+          source = bundler_specs_by_path[engine.root.to_s].source
+          is_path = Bundler::Source::Path === source
+          is_git = Bundler::Source::Git === source
+          development_gem = !is_path || is_git


So, I think I see what you are trying to do, but I think we should go about it differently. From what I can tell you are concerned with just not infecting gems when they live in the Bundler owned gems directory (whether real gem or a git based gem).

So, this can easily be handled by seeing if the plugins path is in bundler's install path. This code should do:

in_bundler_gems = engine.root.expand_path.to_s.start_with?(Bundler.install_path.expand_path.to_s) AssetPath.new(engine, in_bundler_gems) if AssetPath.asset_path?(engine)

Then down below:

@node_modules = in_bundler_gems ? Pathname.new("/tmp/test/#{@namespace}/node_modules") : @path.join('node_modules')

Aaah, yeah that might be better, definitely seems like this involves less poking inside bundler's ..insides :).

Updating.

himdel · 2018-08-09T09:52:00Z

Ok, so looks like we may have the right way of determining which packages should be treated which way.

So for the path then... does manageiq/vendor/node_root make sense?

I'm not sure /tmp is right because it tends to be tmpfs-mounted sometimes, and node_modules is currently about 500M.
But I'm thinking vendor sounds like the place for it.

With the last change:

JS plugins:
  ManageIQ::GraphQL::Engine:
    namespace: manageiq-graphql
    path: /home/himdel/.rbenv/versions/2.4.3/lib/ruby/gems/2.4.0/bundler/gems/manageiq-graphql-5f68621f2791
    node_modules: /home/himdel/manageiq/vendor/node_root/manageiq-graphql/node_modules
    development_gem?: false
  ManageIQ::UI::Classic::Engine:
    namespace: manageiq-ui-classic
    path: /home/himdel/manageiq-ui-classic
    node_modules: /home/himdel/manageiq-ui-classic/node_modules
    development_gem?: true
  ManageIQ::V2V::Engine:
    namespace: manageiq-v2v
    path: /home/himdel/.rbenv/versions/2.4.3/lib/ruby/gems/2.4.0/bundler/gems/miq_v2v_ui_plugin-7b60bd492758
    node_modules: /home/himdel/manageiq/vendor/node_root/manageiq-v2v/node_modules
    development_gem?: false

Fryguy · 2018-08-09T21:44:48Z

vendor sounds perfect. I am going to merge this.

Fryguy

@himdel can you just squash the commits, and then this is good to go.

Fryguy · 2018-08-09T21:50:45Z

lib/vmdb/plugins.rb

@@ -77,7 +77,11 @@ def provider_plugins
    def asset_paths
      @asset_paths ||= begin
        require_relative 'plugins/asset_path'
-        map { |engine| AssetPath.new(engine) if AssetPath.asset_path?(engine) }.compact
+        map do |engine|
+          in_bundler_gems = engine.root.expand_path.to_s.start_with?(Bundler.install_path.expand_path.to_s)


Minor, but this logic can live right in the AssetPath class...then you don't need to modify the params to initialize. Since the information comes from the engine itself, and we are already passing the engine in, then it's not a problem for it to be in there.

Makes sense 👍

…e_root AssetPath now knows about the path and name of each engine But to move node_modules outside of gem directories, we also need to know whether the developer will be doing chances in that particular engine or not - `#development_gem?` assumes anything installed in bundler gems will not get modified. This will enable the ui tasks to use a different node_modules path for gems, no longer polluting the gem dir. Non-development gems will use `manageiq/vendor/node_root/<engine name>/node_modules`, while anything *not* in bundler gems will end up with `node_modules` inside that engine's root. This only provides the info to the appropriate ui rake tasks.

himdel · 2018-08-10T11:09:39Z

Perfect, thanks! :) Squashed now..

miq-bot · 2018-08-10T11:11:49Z

Checked commit https://github.com/himdel/manageiq/commit/34a016a7cbef35316499a9cb4800948e1ffa629d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

miq-bot changed the title ~~Vmdb::Plugins::AssetPath - add node_modules, is_gem~~ [WIP] Vmdb::Plugins::AssetPath - add node_modules, is_gem Aug 8, 2018

miq-bot added the wip label Aug 8, 2018

himdel mentioned this pull request Aug 8, 2018

[WIP] update:ui - support node_modules locations outside of gem engine dirs ManageIQ/manageiq-ui-classic#4435

Closed

Fryguy reviewed Aug 8, 2018

View reviewed changes

Fryguy changed the title ~~[WIP] Vmdb::Plugins::AssetPath - add node_modules, is_gem~~ Vmdb::Plugins::AssetPath - add node_modules, development_gem? Aug 9, 2018

Fryguy approved these changes Aug 9, 2018

View reviewed changes

miq-bot removed the wip label Aug 9, 2018

Fryguy reviewed Aug 9, 2018

View reviewed changes

Fryguy merged commit 5871cb3 into ManageIQ:master Aug 10, 2018

Fryguy added this to the Sprint 92 Ending Aug 13, 2018 milestone Aug 10, 2018

Fryguy added ui enhancement labels Aug 10, 2018

himdel deleted the assetpath-gem branch August 10, 2018 15:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vmdb::Plugins::AssetPath - add node_modules, development_gem? #17818

Vmdb::Plugins::AssetPath - add node_modules, development_gem? #17818

himdel commented Aug 8, 2018 •

edited

Loading

Fryguy Aug 8, 2018

himdel Aug 8, 2018

Fryguy commented Aug 8, 2018

himdel commented Aug 8, 2018 •

edited

Loading

himdel commented Aug 8, 2018

himdel commented Aug 8, 2018

himdel commented Aug 8, 2018

himdel commented Aug 8, 2018 •

edited

Loading

Fryguy Aug 8, 2018 •

edited

Loading

himdel Aug 9, 2018

himdel commented Aug 9, 2018

Fryguy commented Aug 9, 2018

Fryguy left a comment

Fryguy Aug 9, 2018

himdel Aug 10, 2018

himdel commented Aug 10, 2018

miq-bot commented Aug 10, 2018

Vmdb::Plugins::AssetPath - add node_modules, development_gem? #17818

Vmdb::Plugins::AssetPath - add node_modules, development_gem? #17818

Conversation

himdel commented Aug 8, 2018 • edited Loading

Fryguy Aug 8, 2018

Choose a reason for hiding this comment

himdel Aug 8, 2018

Choose a reason for hiding this comment

Fryguy commented Aug 8, 2018

himdel commented Aug 8, 2018 • edited Loading

himdel commented Aug 8, 2018

himdel commented Aug 8, 2018

himdel commented Aug 8, 2018

himdel commented Aug 8, 2018 • edited Loading

Fryguy Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

himdel Aug 9, 2018

Choose a reason for hiding this comment

himdel commented Aug 9, 2018

Fryguy commented Aug 9, 2018

Fryguy left a comment

Choose a reason for hiding this comment

Fryguy Aug 9, 2018

Choose a reason for hiding this comment

himdel Aug 10, 2018

Choose a reason for hiding this comment

himdel commented Aug 10, 2018

miq-bot commented Aug 10, 2018

himdel commented Aug 8, 2018 •

edited

Loading

himdel commented Aug 8, 2018 •

edited

Loading

himdel commented Aug 8, 2018 •

edited

Loading

Fryguy Aug 8, 2018 •

edited

Loading