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

fix: do not run npm --if-present build on npm cache hit #1396

Merged
merged 1 commit into from
Oct 26, 2023

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Oct 26, 2023

Fixes a regression introduced by #1070.

Essentially you can't include ${{ ... }} inside regular YAML if clauses, you have to have the whole clause inside ${{ ... }}.

This explains why every monorepo job builds on every run despite the npm/build cache being hit.

For the situation where the build cache exists (run GH action with debug logging turned on to see this sort of thing):

Before:

##[debug]Evaluating condition for step: 'run'
##[debug]Evaluating: (success() && format('steps.cache.outputs.cache-hit != ''true'' && {0} == ''true''', inputs.build_on_cache_fail))
##[debug]Evaluating And:
##[debug]..Evaluating success:
##[debug]..=> true
##[debug]..Evaluating format:
##[debug]....Evaluating String:
##[debug]....=> 'steps.cache.outputs.cache-hit != ''true'' && {0} == ''true'''
##[debug]....Evaluating Index:
##[debug]......Evaluating inputs:
##[debug]......=> Object
##[debug]......Evaluating String:
##[debug]......=> 'build_on_cache_fail'
##[debug]....=> 'true'
##[debug]..=> 'steps.cache.outputs.cache-hit != ''true'' && true == ''true'''
##[debug]=> 'steps.cache.outputs.cache-hit != ''true'' && true == ''true'''
##[debug]Expanded: (true && 'steps.cache.outputs.cache-hit != ''true'' && true == ''true''')
##[debug]Result: 'steps.cache.outputs.cache-hit != ''true'' && true == ''true'''
##[debug]Starting: run

Essentially it's evaluating (true && 'steps.cache.outputs.cache-hit != ''true'' && true == ''true'''), e.g. (true && string) which always evaluates to true.

After:

##[debug]Evaluating condition for step: 'run'
##[debug]Evaluating: (success() && (steps.cache.outputs.cache-hit != 'true') && (inputs.build_on_cache_fail == 'true'))
##[debug]Evaluating And:
##[debug]..Evaluating success:
##[debug]..=> true
##[debug]..Evaluating NotEqual:
##[debug]....Evaluating Index:
##[debug]......Evaluating Index:
##[debug]........Evaluating Index:
##[debug]..........Evaluating steps:
##[debug]..........=> Object
##[debug]..........Evaluating String:
##[debug]..........=> 'cache'
##[debug]........=> Object
##[debug]........Evaluating String:
##[debug]........=> 'outputs'
##[debug]......=> Object
##[debug]......Evaluating String:
##[debug]......=> 'cache-hit'
##[debug]....=> 'true'
##[debug]....Evaluating String:
##[debug]....=> 'true'
##[debug]..=> false
##[debug]=> false
##[debug]Expanded: (true && ('true' != 'true') && (inputs['build_on_cache_fail'] == 'true'))
##[debug]Result: false

Fixes YAML variable interopolation.
@achingbrain achingbrain requested a review from SgtPooki October 26, 2023 14:17
@achingbrain
Copy link
Member Author

This will shave 2-3 minutes off every linux job in the libp2p monorepo.

@achingbrain
Copy link
Member Author

In this repo too, the npm install/build in CI for this PR took 57s the first time then 16s subsequently.

@achingbrain achingbrain merged commit 7111f49 into master Oct 26, 2023
18 checks passed
@achingbrain achingbrain deleted the fix/cache-modules branch October 26, 2023 17:13
github-actions bot pushed a commit that referenced this pull request Oct 26, 2023
## [41.0.12](v41.0.11...v41.0.12) (2023-10-26)

### Bug Fixes

* do not run `npm --if-present build` on npm cache hit ([#1396](#1396)) ([7111f49](7111f49))
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