https://github.com/theforeman/foreman-packaging/blob/rpm/develop/check_npm_version.sh could probably be modified to check for a specific version rather than just being present. rpmdev-vercmp could be used for version comparison.
I added a check for versions so that it doesn’t change packages that don’t need to be updated.
Calling npm list with --production --no-optional should solve limiting the packages to production ones only.
I’m not sure how great npm2rpm is and if we can accept it’s output blindly. At the moment the script makes significant changes to the spec files that seem to have been created by hand. Anyway here it is and we can start experimenting with it:
https://github.com/theforeman/foreman-packaging/pull/2627
Awesome! The output of npm2rpm has changed quite a bit because I rewrite it. It skips a few things that are no longer needed. Recent specs should already use this, older ones don’t. Cleaning up the situation and finding the right versions to bump to was on my list before branching but I hadn’t gotten around to it yet.
FWIW, the original logic was that we split the packages so that dependencies were the modules we used in the code and thus shipped to users, while devDependencies were the modules that were needed for the build or development process. I guess we could use maybe optionalDependenices for those that aren’t required for build time? sadly it doesn’t look like package.json has any way of grouping modules together other then (ab)using the built in groups.
I went through and re-aligned package.json so that NODE_ENV=production npm install would work properly and only load what’s needed.
On top of that, to help things stay clean and working I am proposing an addition to PR tests to check asset precompile works:
Found a way to reproduce the error on forklift centos7-devel. It’s basically following the same process we do in our packaging. We want to mimic the same thing packaging does:
- First, compile assets on a Foreman without Katello. Let’s call this host “Alice”
- Then on another Foreman with Katello compile assets for Katello. Let’s call this host “Bob”
- On Alice, enable Katello now, but do NOT compile assets for it.
- Copy
katello/public/webpack/katellofrom “Bob” to “Alice”. - Run the foreman server.
In terms of code
- Get a centos7-devel instance in forklift. This will be “Alice”
-
rm -r foreman/public/webpack foreman/node_modulesThis clears everything. - Make sure you DON’T have Katello enabled in
foreman/bundler.d/Gemfile.local.rborforeman/bundler.d/katello.local.rb. You can comment the line in the Gemfile for this. -
npm install(yes, in development, as we wantnode_modules/.bin/webpackto be available) NODE_ENV=production node_modules/.bin/webpack --bail --config config/webpack.config.js- At this point, we have a Foreman installation with webpack assets. You may run
RAILS_ENV=production rails serverif you want to test it. - Get a nightly forklift host, centos7-katello-nightly. This will be “Bob”
- This host will contain the compiled Katello assets at
/usr/share/foreman/public/webpack/katello. Copy them to the devel host. You could do:
# On "bob"
cp -r /usr/share/foreman/public/webpack/katello/ katello
# On your hypervisor, at the forklift dir
scp -r -i .vagrant/machines/centos7-katello-nightly/libvirt/private_key vagrant@centos7-katello-nightly:~/katello .
scp -r katello vagrant@centos7-devel:~/katello_webpack
- SSH to “Alice” again.
- Enable Katello again in the Gemfile, uncommenting the line at
bundler.d/katello.local.rb. - Copy now the Katello assets from “Bob”.
cp -r ~/katello_webpack ~/katello/public/webpack/katello - Go to
https://centos7-devel.yourdomain/redhat_repositories - You’ll see the same error we see on nightlies.
I have not checked if this is the same problem that affects foreman_ansible and foreman_remote_execution.
I also think this problem could be reproduced by creating the Katello webpack assets in any other host, it does not necessarily have to be a nightly host, but I haven’t tried yet. I think this is very likely. In such case, we will have to workaround https://github.com/webpack/webpack/issues/959, https://github.com/webpack/webpack/issues/1155 - thanks @tbrisker for finding these.
I’ll update the thread with any findings.
Confirmed. I can reproduce the same error we get in nightly by compiling the latest Katello webpack assets in a host different to the Foreman host, then sending them over to my Foreman host(TypeError: e[t] is undefined). In this case I was using katello master which includes Fixes #23791 - Upgrade and pin patternfly-react by sharvit · Pull Request #7416 · Katello/katello · GitHub . I have seen multiple messages implying merging that PR would fix nightlies but so far I can’t see that.
At least, right now we have a clear way to quickly reproduce the issue and try out different fixes.
Quick update after two more days of digging into this:
We’ve replaced the webpack HashModuleIdsPlugin with NamedModulesPlugin which lets us see the name of the module that is failing to load instead of the hash.
Now debugging with the latest nightlies, we see katello is asking for a module named ../../../../../../../usr/lib/node_modules/react/index.js, while vendor.js lists that module as ../../../../usr/lib/node_modules/react/index.js. Note the 3 level hierarchy difference between the names - this is why we’re seeing the failures in nightlies on the katello webpack pages.
Next step: identify why katello packages are built from a folder 3 levels deeper than core, leading to the different relative path to the modules. @packaging/@infra - any idea why we’re seeing this?
I’m trying to rebuild the assets on a production installed machine:
cd /usr/share/foreman
ln -s /usr/lib/node_modules
NODE_ENV=production scl enable tfm "webpack --config config/webpack.config.js --bail"
This results in the module identity as ../../lib/node_modules/extract-text-webpack-plugin. This means it always finds the relative path to the module.
When I run the compilation as follows:
cd /usr/lib/node_modules
NODE_ENV=production scl enable tfm "webpack --config /usr/share/foreman/config/webpack.config.js --bail"
Now I get the paths as ./extract-text-webpack-plugin which is probably closer to what we want. It would be consistent between foreman and katello.
After realizing the issue stems from different paths during build, I thought it would be better to use consistent names for the modules that don’t rely on the relative path to the module. To address that I opened:
which simply strips all the ../../../node_modules/ part of the module ID, leaving just the module’s path inside the node_modules folder. This should allow packaging process to run from any path while producing consistent results.
1 Like
I merged the above PR, thanks, it worked wonderfully. As everyone can see on the PR discussion https://github.com/theforeman/foreman/pull/5734#pullrequestreview-131499430 …
The plugin works with any kind of relative paths, and it fixes all of Katello, REX and Ansible!
I am fairly confident that nightlies will be fixed tonight. If they pass and we can check Job Invocations, Ansible reports, and RH subscriptions, we can conclude this thread (thanks to everyone involved!).
Let’s keep in mind we still have other packaging matters to worry about after this:
- debs unable to take webpack assets from plugins (we haven’t even tried AFAIK)
- we should make sure the machine where we build our packages does not matter. e.g: if now, we have a Fedora machine in koji which builds the ‘katello’ package (including webpack assets), and a Centos machine in Koji which builds the ‘foreman’ package, I think it would not work.
1 Like
Due to mock this shouldn’t be an issue. We’ll always have a chroot with the correct OS packages.
Update on the current status:
SimpleNamedModule is in place and seems to have solved the issue for most modules, now for most modules katello manages to find the correct module in vendor.js.
However, two modules katello expects to be in vendor.js aren’t there - warning/warning.js and invariant/browser.js. As far as I can tell, these are being required from react-router.
This might be due to the fact these modules are present in the core foreman node_modules, but possibly nothing in foreman uses them so they are tree-shaken out of vendor.js, while when katello builds they are used and thus remain in vendor.js. This is just a theory at this point though, not anything substantiated yet.
Looking through the Katello staging RPM currently, I see the following in the code for example:
"../../../../../BUILDROOT/tfm-rubygem-katello-3.8.0-.201806261113gitabbff481.nightly.el7.noarch/opt/theforeman/tfm/root/usr/share/gems/gems/katello-3.8.0/webpack/containers/Application/overrides.scss": function(e, t) {},
"../../../../../BUILDROOT/tfm-rubygem-katello-3.8.0-.201806261113gitabbff481.nightly.el7.noarch/opt/theforeman/tfm/root/usr/share/gems/gems/katello-3.8.0/webpack/index.js": function(e, t, o) {
"use strict";
var n = s(o("./webpack/assets/javascripts/react_app/components/componentRegistry.js"))
, r = o("./webpack/assets/javascripts/react_app/common/MountingService.js")
, a = s(o("../../../../../BUILDROOT/tfm-rubygem-katello-3.8.0-.201806261113gitabbff481.nightly.el7.noarch/opt/theforeman/tfm/root/usr/share/gems/gems/katello-3.8.0/webpack/containers/Application/index.js"));
function s(e) {
return e && e.__esModule ? e : {
default: e
}
}
o("../../../../../BUILDROOT/tfm-rubygem-katello-3.8.0-.201806261113gitabbff481.nightly.el7.noarch/opt/theforeman/tfm/root/usr/share/gems/gems/katello-3.8.0/webpack/redux/index.js"),
n.default.register({
name: "xui_katello",
type: a.default
}),
(0,
r.mount)("xui_katello", "#reactRoot")
},
"../../../../../BUILDROOT/tfm-rubygem-katello-3.8.0-.201806261113gitabbff481.nightly.el7.noarch/opt/theforeman/tfm/root/usr/share/gems/gems/katello-3.8.0/webpack/move_to_foreman/Settings/SettingsActions.js": function(e, t, o) {
"use strict";
Object.defineProperty(t, "__esModule", {
value: !0
}),
t.loadSetting = void 0;
var n = o("../../../../../BUILDROOT/tfm-rubygem-katello-3.8.0-.201806261113gitabbff481.nightly.el7.noarch/opt/theforeman/tfm/root/usr/share/gems/gems/katello-3.8.0/webpack/services/api/index.js")
, r = o("../../../../../BUILDROOT/tfm-rubygem-katello-3.8.0-.201806261113gitabbff481.nightly.el7.noarch/opt/theforeman/tfm/root/usr/share/gems/gems/katello-3.8.0/webpack/move_to_foreman/Settings/SettingsConstants.js")
, a = t.loadSetting = function(e) {
return function(t) {
return t({
type: r.GET_SETTING_REQUEST
}),
n.foremanApi.get("/settings/" + e).then(function(e) {
var o = e.data;
t({
type: r.GET_SETTING_SUCCESS,
response: o
})
}).catch(function(e) {
t({
type: r.GET_SETTING_FAILURE,
result: e
})
})
}
}
;
t.default = a
},
"../../../../../BUILDROOT/tfm-rubygem-katello-3.8.0-.201806261113gitabbff481.nightly.el7.noarch/opt/theforeman/tfm/root/usr/share/gems/gems/katello-3.8.0/webpack/move_to_foreman/Settings/SettingsConstants.js": function(e, t, o) {
"use strict";
Note the BUILDROOT pathing artifact which from the RPM build which seems wrong and bad to have it resolving that way and possibly causing errors? I wonder if we need to make those also check for pluginName when generating and cut off the path so its relative to the plugin?
I think as long as its katello calling katello code the buildroot paths are fine - remember, these are just module identifiers, not real paths being used. The paths starting with ./webpack are calls to foreman core modules, which are also identified in the same way in foreman’s bundle.js so we’re good. I think the only case where we may see issues here is if one plugin tries to call another plugin’s js code, which I would argue is a Bad Idea
anyways.
tbrisker https://community.theforeman.org/u/tbrisker Core
June 27I think as long as its katello calling katello code the buildroot paths
are fine - remember, these are just module identifiers, not real paths
being used. The paths starting with ./webpack are calls to foreman core
modules, which are also identified in the same way in foreman’s bundle.js
so we’re good. I think the only case where we may see issues here is if one
plugin tries to call another plugin’s js code, which I would argue is a Bad
Idea[image:] anyways.
unless, a plugin such as foreman tasks add generic components to handle
tasks… 
in that case I would argue the plugin should expose an api allowing other plugins to interact with it, rather then need to include components in it - especially if some day in the future it may even be a completely separate container or something.
With some help from @sharvit, we managed to figure out why katello is expecting these two modules as top-level modules in vendor.js:
This PR added packages for the history, warning, and invariant npm modules, leading them to be installed as top-level modules in /usr/lib/node_modules when building katello. Since webpack finds them there, and some other modules are also using them, it adds them to the vendor.js file that is generated during the katello build (and subsequently discarded), and makes all calls to them from katello.js point to the module in vendor.js, instead of including the nested version from inside the react-router module into katello.js.
Next step, I will open a PR to packaging removing these modules from the packaging (as they are and should be packaged as bundled dependencies of react-router, not separately)
1 Like
PR open:
This was now merged and katello nightlies are working (from the staging repo, there’s still some issues with publishing them).
This PR should make the fix available to 1.18, once that is merged i suggest making RC3 right away:
We also need this PR in 1.18 packaging repo for katello to build correctly:
Once those two are in, all plugins using webpack should be rebuilt to make sure they are using the right module identifiers.
2 Likes