Test and improve the merging of package configuration patches #348
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
I recently looked into this when trying to update the `config` crate and noticed that the "merging" is more of an anti-feature (IMO). This demonstrates the surprising behavior of the extra check based on the file/base name of the patch. Signed-off-by: Michael Weiss <michael.weiss@eviden.com>
The old logic silently ignored a patch if a patch with the same name was
declared in an upper level/layer package configuration.
This fixes that issue and I now know why that "else if" branch exists:
The `config.set_once("patches", config::Value::from(patches))` didn't
last beyond the next `config.merge()` so the custom logic is required to
ensure that `patches` will be empty so that `patches_before_merge` (with
the value from `config.set_once()`) gets used instead.
I'm now using `config.merge()` to properly update `patches` so that we
can simply check if `patches_before_merge == patches` (i.e., no changes
occurred after merging the next `pkg.toml` file).
This looks a bit more complex at first but can be cleaned up next and I
hope that newer versions of the `config` crate make this easier for us
(we can really simplify the code if we can request the source (file path
of the `pkg.toml` file) that set the final `patches` value).
I can also "fix" the tests now that the "merging" works as intended.
Signed-off-by: Michael Weiss <michael.weiss@eviden.com>
This keeps the logic from the previous commit but restructures/reorders the code to make it much more readable. Signed-off-by: Michael Weiss <michael.weiss@eviden.com>
Merged
via the queue into
science-computing:master
with commit Feb 21, 2024
c4e7156
13 checks passed
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 adds test to ensure the additional logic for handling/merging the
patchesvalue (array) inpkg.tomlworks correctly, simplifies the code (especially to make it more readable / easier to understand), and fixes a bug (minor as it's a special case).Edit: The bugfix could be considered a bit of a breaking change but I consider it self-documenting as it results in a easy to understand error message if invalid (non-existent) patches are specified, e.g. (I added the
pkg.tomlfile path to the output):