Skip to content
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

Add compression-webpack-plugin #1453

Conversation

tbrisker
Copy link
Member

This depends on theforeman/foreman#3883 being merged.

For plugin updates, please indicate which repos this should be built into:

  • Nightly
  • 1.14
  • 1.13
  • 1.12

See Foreman's plugin maintainer documentation for more information.


@lzap
Copy link
Member

lzap commented Jan 2, 2017

FYI core was merged.

@domcleal
Copy link
Contributor

domcleal commented Jan 3, 2017

This is still failing to build, please fix the failure and ensure comments/FIXME comments are resolved in the spec file.

@tbrisker
Copy link
Member Author

tbrisker commented Jan 3, 2017

@domcleal updated the spec file, hopefully tests will pass now, thanks.

@tbrisker tbrisker force-pushed the rpm/compression-webpack-plugin branch 2 times, most recently from 9934c7c to 1c3b47f Compare January 4, 2017 11:28
@tbrisker
Copy link
Member Author

tbrisker commented Jan 4, 2017

@domcleal fixed the failing build, seems to work now.

@domcleal
Copy link
Contributor

domcleal commented Jan 4, 2017

The spec file lists a large number of bundled packages, but the resulting RPM doesn't seem to contain any node_modules directory - only the top-level package contents:

/usr/lib/node_modules/compression-webpack-plugin
/usr/lib/node_modules/compression-webpack-plugin/.gitattributes
/usr/lib/node_modules/compression-webpack-plugin/.npmignore
/usr/lib/node_modules/compression-webpack-plugin/README.md
/usr/lib/node_modules/compression-webpack-plugin/index.js
/usr/lib/node_modules/compression-webpack-plugin/package.json

Please check the dependencies are present.

@tbrisker
Copy link
Member Author

tbrisker commented Jan 4, 2017

looks like this is pulling in a ton of unwanted optional dependencies due to an issue with how npm registry lists dependcies. npm/npm-remote-ls#32 should fix it and significantly reduce the size of this package, I'll try to get this fixed.

@ehelms
Copy link
Member

ehelms commented Jan 9, 2017

@tbrisker is the npm-remote-ls fix required to generate the package or to build the package? If the former, could we build out the spec and dependencies by hand so as to get this built given it will be required to get nightly builds going.

@dLobatog
Copy link
Member

@tbrisker I think this could be built manually as 3 packages with the single strategy, it'd save us time until they merge the npm-remote-ls fix.

@ehelms It's not strictly required but without it, the bundle package will be huge, when there are only 3 mandatory dependencies in fact.

@tbrisker
Copy link
Member Author

Pushed again with only the required deps using the single strategy, @dLobatog @domcleal please re-review.

@dLobatog
Copy link
Member

@tbrisker This is missing adding the package to foreman.spec

@@ -29,13 +29,15 @@
<packagereq type="default">foreman-selinux</packagereq>
<packagereq type="default">foreman-sqlite</packagereq>
<packagereq type="default">foreman-vmware</packagereq>
<packagereq type="default">nodejs-async</packagereq>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run ./comps_doc.sh, there are no -doc packages in either of the comps

@domcleal
Copy link
Contributor

webpack-sources appears to depend on source-map and source-list-map, but these dependencies aren't either here or bundled. Can you check the package deps resolve?

Copy link
Contributor

@domcleal domcleal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nodejs-webpack-sources has unresolved deps.

@tbrisker
Copy link
Member Author

@domcleal Thanks, updated with those 2 packages as well.

%if 0%{?enable_tests}
%check
%{nodejs_symlink_deps} --check
#$CHECK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useless comment.

Version: 0.5.6
Release: 1%{?dist}
Summary: Generates and consumes source maps
License: BSD-3-Clause
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BSD

%if 0%{?enable_tests}
%check
%{nodejs_symlink_deps} --check
#$CHECK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

%files
%{nodejs_sitelib}/%{npm_name}
%doc LICENSE
%doc CHANGELOG.md
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-doc subpackage

@@ -29,13 +29,15 @@
<packagereq type="default">foreman-selinux</packagereq>
<packagereq type="default">foreman-sqlite</packagereq>
<packagereq type="default">foreman-vmware</packagereq>
<packagereq type="default">nodejs-async</packagereq>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nodejs-async 1.5's already in the distribution, is the older dep going to resolve correctly, since it's 0.2.x? Bundling the dependency would be preferable I think in this case.

@tbrisker
Copy link
Member Author

Yet another try, back to bundled.

@domcleal
Copy link
Contributor

Please check the RPM builds, there is a failure.

@tbrisker tbrisker force-pushed the rpm/compression-webpack-plugin branch from 6d6010a to ba33463 Compare January 23, 2017 11:47
@tbrisker
Copy link
Member Author

I give up. The time and effort wasted on making these useless rpms is ridiculous, instead of just running an npm install and adding node_modules to the sources passed to koji prior to build.
Anyone else who want to get this building is welcome to, I'm not going to sink any more hours into figuring out why this fails to build.

@domcleal domcleal closed this Jan 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants