Drop NPM < 7 support, SCL support and use a relative cache directory#87
Drop NPM < 7 support, SCL support and use a relative cache directory#87ekohl wants to merge 4 commits intotheforeman:masterfrom
Conversation
| %prep | ||
| mkdir -p %{npm_cache_dir} | ||
| for tgz in %{sources}; do | ||
| echo $tgz | grep -q registry.npmjs.org || npm cache add --cache %{npm_cache_dir} $tgz |
There was a problem hiding this comment.
While it does not hurt to keep this logic, I'd suggest removing it and adding all sources to the cache as --offline hard fails if the package is missing from the cache.
There was a problem hiding this comment.
That will lead to much bigger cache tarballs that we need to include in our git tree, which blows it up over time. It's already bad, but that makes it way worse. See #68 / https://community.theforeman.org/t/trimming-foreman-packaging-branches/29189/7 for details but the tl;dr is that I saw a 31% reduction in the cache filesize on the specific package I tested.
There was a problem hiding this comment.
And since I perhaps missed it: you do need the tarball with the cached responses because that's how NPM works. That should not be added to NPM's cache. I'll admit it's not easy to read due to the ||. It effectively comes down to:
if [[ $tgz != *registry.npmjs.org* ]]; then
npm cache add --cache %{npm_cache_dir} $tgz
fiBut I don't know if you can use bashisms in RPM.
| @@ -36,7 +36,7 @@ done | |||
| %setup -T -q -a {{{PROVIDES.length}}} -D -n %{npm_cache_dir} | |||
|
|
|||
| %build | |||
| npm install {{{LEGACY_PEER_DEPS}}}--offline --cache %{npm_cache_dir} --package-lock false --omit optional --install-strategy shallow %{npm_name}@%{version} | |||
| npm install {{{LEGACY_PEER_DEPS}}}--offline --cache %{_builddir}/%{npm_cache_dir} --package-lock false --omit optional --install-strategy shallow %{npm_name}@%{version} | |||
There was a problem hiding this comment.
Could we define npm_cache_path once as a full path relative to builddir?
%define npm_cache_path %{_builddir}/npm_cache_dir
There was a problem hiding this comment.
Possibly, I didn't investigate too deeply.
ogajduse
left a comment
ogajduse
left a comment
There was a problem hiding this comment.
I am very new to this project, but from what I've seen, there is a portion of breaking changes. Do you plan to release a new major version?
Yes, this is IMHO major version material. |
|
What is the state of this PR? I have used it to generate theforeman/foreman-packaging#13080 and IMHO, it looks good. It would be nice to continue here as ALL nodejs packages in foreman-packaging still have the SCL logic included. |
On EL10 this is needed to properly copy license and doc files.
ekohl
changed the title
|
@ogajduse Do you mind taking another look? |
3 participants
See individual commits for details.