Use Make conditionals #1168
Merged
Use Make conditionals #1168
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead of using shell conditionals (`if [ $(IMP) = true ]; then ... ; fi`) in all targets of the imports section, bracket the entire section in a single Make conditional.
Some pre-existing Make conditionals were using the following form: ifeq ($(strip $(VARIABLE),true)) This is because some VARIABLES were declared with a comment on the same line, as in VARIABLE = true # some comment The presence of the comment causes Make to treat the whitespaces on the line as significant, meaning that VARIABLE in the example above has the value " true ", instead of "true". Using the `strip` function in the condition allowed to test against the intended value of the variable, rather than the value effectively set. But it is actually better to fix the declaration of the variable (moving the comment to the line above), once and for all, than to strip whitespaces whenever the variable is tested. This has been done for all affected variables, so the calls to `strip` are no longer necessary.
Apply to targets of the components pipeline the same principle as for mirrors and imports: bracket the entire section within a single Make conditional on the COMP variable. Also, use a Make conditional on the MIR variable to avoid re-generating a component that is downloaded from a remote source when MIR is false.
The general rule is that when MIR is set to false, nothing should ever be fetched from a remote source -- pipelines should only use locally available resources. So if a mapping set is defined as having a remote source, we bracket the rule that downloads that set in a Make conditional on MIR.
Apply to the DOSDP pipeline targets the same principle as for the imports, mirrors, and components pipelines: bracket the entire section in a Make conditional on the PAT variable.
It shouldn't be necessary to have a rule that does nothing but display a message that says that nothing is being done.
I missed the fact that this rule is supposed to behave differently depending on whether the pattern pipeline is enabled or not. So in this particular instance we need two rules: one in the block where PAT is true, and one in the alternative block.
matentzn
reviewed
Jan 14, 2025
Declare all the variables that control whether workflows are enabled (MIR, IMP/IMP_LARGE, COMP, and PAT) in a single block near the beginning of the Makefile. This both ensures that all such variables are always declared before they are used, and makes it easier for users to find them and understand what they are for.
matentzn
approved these changes
Jan 15, 2025
|
Regarding the need for testing: I’d say that it can be done as part of the normal testing we do prior to a major release (as outlined here). During development, it’s OK to rely mostly on the CI-based testing + code reviews for most PRs. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR updates the standard Makefile template to replace, wherever possible, shell conditionals by Make conditionals.
That is, rules like the following:
is replaced by
closes #912