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

Unify asset building and use find_program to find NPM #2522

Merged
merged 2 commits into from
May 12, 2024

Conversation

chewi
Copy link
Contributor

@chewi chewi commented May 11, 2024

Description

Firstly, this unifies the asset building by leveraging CMake's env command. add_custom_target can also accept multiple commands to be run in sequence. It isn't clear how quoting applies here, but I've tested it, and it seems to be behave as expected when spaces are present.

Secondly, this uses find_program to find NPM so it can be overridden. This is useful for Gentoo, which needs to be able to do entirely offline builds, because it can override this with /bin/true while shipping pre-compiled assets. Gentoo has tried to ship cached NPM modules instead, but it turns out these are very sensitive to the NPM version. I also tried swapping in Yarn, which handles caching better, but ran into other issues.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Dependency update (updates to dependencies)
  • Documentation update (changes to documentation)
  • Repository update (changes to repository files, e.g. .github/...)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

Branch Updates

LizardByte requires that branches be up-to-date before merging. This means that after any PR is merged, this branch
must be updated before it can be merged. You must also
Allow edits from maintainers.

  • I want maintainers to keep my branch updated

Copy link

codecov bot commented May 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 7.00%. Comparing base (4e49db9) to head (4c44738).

Additional details and impacted files
@@            Coverage Diff             @@
##           nightly   #2522      +/-   ##
==========================================
- Coverage     7.02%   7.00%   -0.03%     
==========================================
  Files           87      87              
  Lines        17675   17669       -6     
  Branches      8395    8395              
==========================================
- Hits          1242    1238       -4     
- Misses       13709   13710       +1     
+ Partials      2724    2721       -3     
Flag Coverage Δ
Linux 5.42% <ø> (+0.05%) ⬆️
Windows 2.60% <ø> (ø)
macOS-12 ?
macOS-13 ?
macOS-14 8.28% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 5 files with indirect coverage changes

chewi added 2 commits May 12, 2024 11:39
`add_custom_target` can also accept multiple commands to be run in
sequence. It isn't clear how quoting applies here, but I've tested it,
and it seems to be behave as expected when spaces are present.
This is useful for Gentoo, which needs to be able to do entirely offline
builds, because it can override this with `true` while shipping
pre-compiled assets. Gentoo has tried to ship cached NPM modules
instead, but it turns out these are very sensitive to the NPM version.
@ReenigneArcher ReenigneArcher merged commit 6674090 into LizardByte:nightly May 12, 2024
51 of 53 checks passed
KuleRucket pushed a commit to KuleRucket/Sunshine that referenced this pull request Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants