Fix some layers inconsistencies #589
Merged
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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #589 +/- ##
==========================================
+ Coverage 88.88% 88.89% +0.01%
==========================================
Files 48 48
Lines 15270 15339 +69
==========================================
+ Hits 13572 13636 +64
- Misses 1698 1703 +5 ☔ View full report in Codecov by Sentry. |
|
@rhubert It would be good if you could test this on your side... |
|
I can't test this before 21. October...
04.10.2024 22:43:15 Jan Klötzke ***@***.***>:
…
@rhubert[https://github.com/rhubert] It would be good if you could test this on your side...
—
Reply to this email directly, view it on GitHub[#589 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAHEZJSF226XJ7POK5MRHW3ZZ34WFAVCNFSM6AAAAABPMRPV4WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJUGUZTOOBQHE].
You are receiving this because you were mentioned.
[Verfolgungsbild][https://github.com/notifications/beacon/AAHEZJULJNZV42NABSIB2C3ZZ34WFA5CNFSM6AAAAABPMRPV4WWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTUOXG7VC.gif]
|
The layers do not use the regular user configurations files at all. It thus makes no sense to accept them either.
No need to parse after an update because the result won't be used anyway.
This was accidentally introduced in f70beb1.
Move the environment whitelist default set to the utils module so that it can be reused.
No need to recalculate it all the time.
We always check for the existence of a yaml file before where necessary.
Makes it possible to use it in a "with" expression.
Make it a class method so that it can be used independently.
Make it a class method so that it can be used independently.
The handling of layers just depends on their config.yaml. Actually, only a subset of the project configuration is needed. Instead of using the regular RecipeSet parsing function with an early exit, do all config.yaml processing in the layers module itself. This fixes a couple of subtle corner cases that so far got unnoticed: * We still parsed the system- and user-global default.yaml configuration files. The data from these files was not used except the URM SCM mirrors. Now everything depends just on the layer configuration files. * The layersScmOverrides and layersWhitelist keys were only appended when traversing the layers. This causes settings of unrelated layers to affect each other. Now the layer settings are handled strictly hierarchical. * It was possible to switch to the "import" SCM via layersScmOverrides. This is prevented now by always using the layers SCM schema. * There were no default environment white list entries. This was inconsistent to the regular white list handling.
Now that the layers logic handles all settings itself, the noLayers early parsing exit can be removed.
Layer configuration files on the command line should have the highest precedence. If multiple files are passed, later ones should have a higher precedence. Settings in layer config.yaml files affect only their sub-layers. The SCM overrides of a layer have a higher precedence than the SCM overrides of a layer beneath it. This ensures that higher layers (or the command line configuration files) can override and SCMs beneath it.
The event loop is needed when doing something. It is not part of the layers state.
No need to use long, absolute paths. The current working directory will not change so there is no need to prepend it. This change is incompatible to existing workspace states! The workspace state saved the absolute path previously which won't match now.
Always parse the action first. The available command line options actually depend on the action.
Get feature parity with the "bob status" command in this respect.
jkloetzke
force-pushed
the
polish-layers
branch
2 times, most recently
from
6c697c4 to
31f84ee
Compare
|
Ping @rhubert. I have to admit this got quite large. 🙈 Anyway, could you give it a try? I would like to make the 0.25 release once this has been merged... |
|
Sorry for the long delay. I had some days off. I'm not sure if it makes sense to review your changes in detail but I made a few tests this morning. First thing I noticed is that Anyway, I removed the old looks as expected now but immediately move the layers to attic after checkout?
I can reproduce the move-to-attic-after-checkout behavior by deleting the layers dir and switch back to Also note the absolute attic-layer-path in your version compared to master. Not sure if this is intended... |
|
After looking into your commits I think the findings are expected. But bash-completion need some polishing as well: diff --git a/contrib/bash-completion/bob b/contrib/bash-completion/bob
index f48a4f0..3565ca9 100644
--- a/contrib/bash-completion/bob
+++ b/contrib/bash-completion/bob
@@ -388,12 +388,12 @@ __bob_status()
__bob_layers_status()
{
- __bob_complete_path "-c -D -v -lc --attic --no-attic"
+ __bob_complete_words "-D -v -lc --show-clean --show-overrides"
}
__bob_layers_update()
{
- __bob_complete_path "-c -D -v -lc --attic --no-attic"
+ __bob_complete_words "-D -v -lc --attic --no-attic"
}
__bob_layers()(Maybe even more as |
|
layersWhitelist is also not working (same on master 😭 ) Fix: diff --git a/pym/bob/layers.py b/pym/bob/layers.py
index 26ab5fb..bc5cee7 100644
--- a/pym/bob/layers.py
+++ b/pym/bob/layers.py
@@ -26,7 +26,7 @@ class LayersConfig:
ret.__policies = self.__policies
ret.__whiteList.update([c.upper() if self.__platform == "win32" else c
- for c in config.get("layersWhiteList", []) ])
+ for c in config.get("layersWhitelist", []) ])
ret.__scmOverrides[0:0] = [ ScmOverride(o) for o in config.get("layersScmOverrides", []) ]
if rootLayer: |
-> diff --git a/pym/bob/input.py b/pym/bob/input.py
index 9922a0b..1b6b8d7 100644
--- a/pym/bob/input.py
+++ b/pym/bob/input.py
@@ -2032,7 +2032,7 @@ class Recipe(object):
self.__layer = layer
sourceName = ("Recipe " if isRecipe else "Class ") + packageName + (
- ", layer "+"/".join(layer) if layer else "")
+ ", layer "+ layer if layer else "")
incHelperBash = IncludeHelper(BashLanguage, recipeSet.loadBinary,
baseDir, packageName, sourceName).resolve
incHelperPwsh = IncludeHelper(PwshLanguage, recipeSet.loadBinary,
@@ -3567,7 +3567,7 @@ class RecipeSet:
minVer = config.get("bobMinimumVersion", "0.16")
if compareVersion(maxVer, minVer) < 0:
raise ParseError("Layer '{}' requires a higher Bob version than root project!"
- .format("/".join(layer)))
+ .format(layer))
maxVer = minVer # sub-layers must not have a higher bobMinimumVersion
# Determine policies. The root layer determines the default settings
@@ -3577,7 +3577,7 @@ class RecipeSet:
for (name, behaviour) in config.get("policies", {}).items():
if bool(self.__policies[name][0]) != behaviour:
raise ParseError("Layer '{}' requires different behaviour for policy '{}' than root project!"
- .format("/".join(layer), name))
+ .format(layer, name))
else:
self.__policies = self.calculatePolicies(config) |
Co-authored-by: Ralf Hubert <[email protected]>
This actually caused the layer digest to be saved as tuple rather than the digest string itself.
jkloetzke
force-pushed
the
polish-layers
branch
from
31f84ee to
73bde4a
Compare
No worries. My fault. I somehow overlooked that you have already written that you're not able to look into it until now. Anyway, thanks for testing the PR! I don't think it makes sense to review each commit in detail.
Indeed. The behaviour is caused by the state storing now the relative path instead of the absolute path. So far, only relative paths are stored in the project state and I wanted to align this for the layers too.
Thanks! I've picked up your fixes and added them to the stack. |
|
Thanks. I did some more testing and took this also to our CI to test the diff --git a/pym/bob/layers.py b/pym/bob/layers.py
index bc5cee7e..99f4be59 100644
--- a/pym/bob/layers.py
+++ b/pym/bob/layers.py
@@ -2,13 +2,14 @@ import datetime
import os
import schema
import shutil
+from textwrap import indent
from .errors import BuildError
from .invoker import CmdFailedError, InvocationError, Invoker
from .scm import getScm, ScmOverride, ScmStatus, ScmTaint
from .state import BobState
from .stringparser import Env
from .input import RecipeSet, Scm, YamlCache
-from .utils import INVALID_CHAR_TRANS, getPlatformString, getPlatformEnvWhiteList, compareVersion
+from .utils import INVALID_CHAR_TRANS, getPlatformString, getPlatformEnvWhiteList, compareVersion, joinLines
from .tty import DEBUG, EXECUTED, INFO, NORMAL, IMPORTANT, SKIPPED, WARNING, log
class LayersConfig: |
The A (attic), N (new) and O (overridden) flags need to be calculated separately.
Switching a layer SCM is just like doing a fresh checkout. Use the same severity.
The setting if obsolete layers shall be moved to the attic is not a property of the layer but a tunable of the checkout function.
To stop traversing at layers without SCM is dangerous. Such layers can still hold a reference other layers by means of an SCM.
Usually, the project tree (where the recipes are located) and the build
tree (dev/work directory) are in the same directory. But the build
directory can be separated by using "bob init".
In this case, layers that are backed by an SCM, should go to the build
directory. There are two main reasons for this:
1. There can be multiple build directories per project directory. Each
build directory is managed individually and thus the layers can be
updated independently from each other. Doing that in the common
project directory would create conflicts.
2. The state of the layers is stored in the build directory.
Consequently, there is no common state that would describe the
layer SCMs in the project tree.
Fixes BobBuildTool#590.
Fix the typo when fetching layersWhitelist. Co-authored-by: Ralf Hubert <[email protected]>
There were some remnants of the removed nested layers when generating error messages. Addendum to 6f84d58. Co-authored-by: Ralf Hubert <[email protected]>
jkloetzke
force-pushed
the
polish-layers
branch
from
73bde4a to
cbd8d23
Compare
|
Thanks for the patch! |
|
I haven't found any other issue and all my tests+use cases are working as expected :) Thanks for "polishing" this 👍 |
|
Thanks for testing. 👍 Next step, 0.25 release... 😉 |
|
Hmm...the attic handling is broken... attic is None in this case if not attic:
raise BuildError("Layer '{}' inline switch not possible and move to attic disabled '{}'!"
.format(self.__name, self.__layerDir)) |
|
Grmpf. Would you mind creating a PR? |
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 fixes a couple of subtle corner cases that so far got unnoticed:
configuration files. The data from these files was not used except
the URM SCM mirrors. Now everything depends just on the layer
configuration files.
appended when traversing the layers. This causes settings of
unrelated layers to affect each other. Now the layer settings are
handled strictly hierarchical.
This is prevented now by always using the layers SCM schema.
inconsistent to the regular white list handling.
Additionally, it improves the following things: