-
Notifications
You must be signed in to change notification settings - Fork 170
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
Calculate preprocessor symbols #2439
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Left a comment that implies that cleanModePreprocessorSymbols setting might not be needed anymore.
I suppose it's required by AL-Go.
Can we instead set it so that cleanModePreprocessorSymbols is updated every time there is a major version change? What do you think?
@@ -53,6 +53,8 @@ if($appType -eq 'app') | |||
else { | |||
Compile-AppInBcContainer @tempParameters | Out-Null | |||
} | |||
|
|||
$parameters.Value["preprocessorsymbols"] = Get-PreprocessorSymbols |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the clean tags are set here, what's the point of cleanModePreprocessorSymbols setting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None. They're not used. But AL-Go fails if we don't set anything.
Maybe we should just write a warning here instead of throwing an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a defensive check, so not sure about a warning. How would you fix such a warning?
But why automation PRs? 🤔
My idea was just to add a check (in "Verify App Changes" maybe) that the cleanModePreprocessorSymbols
are in order, based on the current version.
Then, to be on the safe side, Get-PreprocessorSymbols
can dynamically add CLEAN preprocessor symbols for all versions between the current version and the version on main.
Generally speaking, CLEAN26 makes no sense in version 25. But so do all the clean builds we do all around, but main.
Co-authored-by: Maria Zhelezova <[email protected]>
Co-authored-by: Maria Zhelezova <[email protected]>
I don't hate the idea, but it does imply more automation PRs and for that reason I'm hesitant. |
Closing for now |
Summary
Calculate CLEAN tags instead of using list
Work Item(s)
Fixes AB#559429