You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@buildstream.apache.org by GitBox <gi...@apache.org> on 2021/12/06 15:21:40 UTC
[GitHub] [buildstream] BuildStream-Migration-Bot opened a new issue #1360: `notparallel` variable is broken
BuildStream-Migration-Bot opened a new issue #1360:
URL: https://github.com/apache/buildstream/issues/1360
[See original issue on GitLab](https://gitlab.com/BuildStream/buildstream/-/issues/1360)
In GitLab by [[Gitlab user @willsalmon]](https://gitlab.com/willsalmon) on Jul 10, 2020, 10:39
## Summary
The `notparallel` variable dose not seem to force `MAKEFLAGS: -j1` in the environment
This means that projects that have bugs associated with `make -j2` can not be built, eg slang.
Please see the failure: https://gitlab.com/celduin/bsps/bst-boardsupport/-/jobs/625698066
The variable being incorrectly set on line 21 in the log: https://gitlab.com/celduin/bsps/bst-boardsupport/-/jobs/625698066/artifacts/file/artifact/freedesktop-sdk/components-slang/1ad3d1df-build.3146.log
The element https://gitlab.com/freedesktop-sdk/freedesktop-sdk/-/blob/master/elements/components/slang.bst showing the:
```
variables:
builddir: ''
notparallel: true
```
The problem is further compounded by the fact that bst will not let you set `MAKEFLAGS` in the environment section as it is `protected`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
gtristan commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-1020781095
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan edited a comment on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
gtristan edited a comment on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-1015019700
Maybe the lines of code in master have shifted since this comment, my 2834 says:
```python
self.__environment = unexpanded_env.strip_node_info()
```
> So the above theory is correct: expanding the variables in https://github.com/apache/buildstream/blob/master/src/buildstream/element.py#L2834 changes the value of the class variable `__defaults`.
I'll try to summarize what *should* be happening here in `__initialize_from_yaml()` (which is outlined [here in the docs](https://docs.buildstream.build/master/format_intro.html#composition)):
* `self.__init_defaults()` will initialize the **class** variable `__defaults`, this is a composition of project level overrides on top of plugin provided defaults
* Questionably this composition should also be composited on top of `project.base_variables`, I think the reason why we cannot immediately cache `builtin -> project.conf -> plugin -> project.conf overrides` is because of the dual-pass loading of `project.conf` where the first pass is not a complete load.
* `self.__extract_variables()` will compose the variables
* The previously cached `default_vars` is loaded from the **class** variable `__defaults`
* The `default_vars` (which is only plugin + overrides) is composited *on top* of the `project.conf` vars
* The `element_vars` is composited on the very top of this composition and has the *last word*
* Now that we have a fully sane composition of the variables `MappingNode` for this particular element instance, the `Variables()` object instance is created for this element instance
* This is where the `notparallel` magic happens, forcing the `max-jobs` variable to `1`, this seems like the right time to do this to me.
* `self.__extract_environment()` will now compose the environment following similar rules
* The previously cached `default_env` is loaded from the **class** variable `__defaults`
* The `default_env` (which is only plugin + overrides) is composited *on top* of the `project.conf` environment
* The `element_env` is composited on the very top of this composition and has the *last word*
* Now the previously created `Variables()` instance is used to expand variables in the fully composited `unexpanded_env`, at this point the `Variables()` instance should have `max-jobs` set to `1` already and we should have the correct `MAKEFLAGS`
> The line responsible for this is `default_env._composite(environment)` as defined in [`Element.__extract_environment()`](https://github.com/apache/buildstream/blob/0df1bfc4e27055a7974bafb40461208bf4283f26/src/buildstream/element.py#L2941). The following change fixes the issue, though I'm not sure if it's the right approach or if the `_composite()` method should be modified instead.
If your patch does indeed change something, then I think you've indeed found a bug in `MappingNode.composite()`, this function should modify the *target* node only and should not cause residual side effects in the *source* node.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
gtristan commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-1028721436
> Here you go: https://github.com/abderrahim/incorrect-max-jobs
>
> `bst show --format %{env} parallel.bst` and `bst show --format %{env} notparallel.bst` will show the correct MAXJOBS variable. but `bst show --format %{env} parallel.bst notparallel.bst` will show the same value for both elements.
I've provided an isolated test case for this in the https://github.com/apache/buildstream/tree/tristan/test-notparallel branch now.
To observe the broken behavior, you can type `tox -- tests/format/variables.py::test_notparallel_twice`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan edited a comment on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
gtristan edited a comment on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-1028721436
> Here you go: https://github.com/abderrahim/incorrect-max-jobs
>
> `bst show --format %{env} parallel.bst` and `bst show --format %{env} notparallel.bst` will show the correct MAXJOBS variable. but `bst show --format %{env} parallel.bst notparallel.bst` will show the same value for both elements.
I've provided an isolated test case for this in the https://github.com/apache/buildstream/tree/tristan/test-notparallel branch now (#1570).
To observe the broken behavior, you can type `tox -- tests/format/variables.py::test_notparallel_twice`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
abderrahim commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-1015097547
Yup, lines have shifted. The line in question is
```python
self.__variables.expand(unexpanded_env)
```
So what happens is that `Node._composite()` does a shallow copy of the nodes, then `Variables.expand()` modifies them in-place, resulting in a modified `__defaults` class variable. which is then composited into other elements with the wrong value.
Here is a minimal reproducer for the issue:
```python
from buildstream.node import Node
from buildstream._variables import Variables
default = Node.from_dict({'MAKEFLAGS': '-j%{max-jobs}'})
element = Node.from_dict({'name': 'my-element.bst'})
print(default, element, sep='\n')
# [synthetic node]: {'MAKEFLAGS': '-j%{max-jobs}'}
# [synthetic node]: {'name': 'my-element.bst'}
default._composite(element)
print(default, element, sep='\n')
# [synthetic node]: {'MAKEFLAGS': '-j%{max-jobs}'}
# [synthetic node]: {'name': 'my-element.bst', 'MAKEFLAGS': '-j%{max-jobs}'}
v = Variables(Node.from_dict({'max-jobs': '1'}))
v.expand(element)
print(default, element, sep='\n')
# [synthetic node]: {'MAKEFLAGS': '-j1'}
# [synthetic node]: {'name': 'my-element.bst', 'MAKEFLAGS': '-j1'}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim edited a comment on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
abderrahim edited a comment on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-1015097547
Yup, lines have shifted. The line in question is
```python
self.__variables.expand(unexpanded_env)
```
So what happens is that `Node._composite()` does a shallow copy of the nodes, then `Variables.expand()` modifies them in-place, resulting in a modified `__defaults` class variable. which is then composited into other elements with the wrong value.
Here is a minimal reproducer for the issue:
```python
from buildstream.node import Node
from buildstream._variables import Variables
default = Node.from_dict({'MAKEFLAGS': '-j%{max-jobs}'})
element = Node.from_dict({'name': 'my-element.bst'})
v = Variables(Node.from_dict({'max-jobs': '1'}))
print(default, element, sep='\n')
# [synthetic node]: {'MAKEFLAGS': '-j%{max-jobs}'}
# [synthetic node]: {'name': 'my-element.bst'}
default._composite(element)
print(default, element, sep='\n')
# [synthetic node]: {'MAKEFLAGS': '-j%{max-jobs}'}
# [synthetic node]: {'name': 'my-element.bst', 'MAKEFLAGS': '-j%{max-jobs}'}
v.expand(element)
print(default, element, sep='\n')
# [synthetic node]: {'MAKEFLAGS': '-j1'}
# [synthetic node]: {'name': 'my-element.bst', 'MAKEFLAGS': '-j1'}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan edited a comment on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
gtristan edited a comment on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-986390997
> I can reproduce this issue, can someone reopen this or should I file a new one?
This feels unlikely, with which plugin are you able to reproduce? It is the plugin YAML which is responsible for setting `MAKEFLAGS` according to `%{max-jobs}` (but that is already mentioned in Chandan’s comment).
Is `%{max-jobs}` not being setup properly in the core ?
Or, is it possible you are confusing the role or `notparallel` with the role of the `—builders` cli option ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
gtristan commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-1020935348
> Maybe composite itself could be unittested to get it to break?
Possible but I'm really more interested in reproducing the issue with an end-to-end test, I'd rather keep internal unit testing down to a minimum if and when it is at all possible to reproduce a bug in action.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
abderrahim commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-987000573
So the above theory is correct: expanding the variables in https://github.com/apache/buildstream/blob/master/src/buildstream/element.py#L2834 changes the value of the class variable `__defaults`.
The line responsible for this is `default_env._composite(environment)` as defined in [`Element.__extract_environment()`](https://github.com/apache/buildstream/blob/0df1bfc4e27055a7974bafb40461208bf4283f26/src/buildstream/element.py#L2941). The following change fixes the issue, though I'm not sure if it's the right approach or if the `_composite()` method should be modified instead.
```diff
diff --git a/src/buildstream/element.py b/src/buildstream/element.py
index ee3b2fc75..b1c8d16a0 100644
--- a/src/buildstream/element.py
+++ b/src/buildstream/element.py
@@ -2947,7 +2947,7 @@ class Element(Plugin):
else:
environment = project.base_environment.clone()
- default_env._composite(environment)
+ default_env.clone()._composite(environment)
element_env._composite(environment)
environment._assert_fully_composited()
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
abderrahim commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-1016795288
I just stumbled upon this old MR: https://gitlab.com/BuildStream/buildstream/-/merge_requests/1929
It seems to suggest we're indeed supposed to use `Node.clone()` with composition.
CC: @BenjaminSchubert
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] nanonyme commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
nanonyme commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-1020860097
Maybe composite itself could be unittested to get it to break?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
abderrahim commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-1020867878
Here you go: https://github.com/abderrahim/incorrect-max-jobs
`bst show --format %{env} parallel.bst` and `bst show --format %{env} notparallel.bst` will show the correct MAXJOBS variable. but `bst show --format %{env} parallel.bst notparallel.bst` will show the same value for both elements.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
abderrahim commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-986807569
It seems to be that `max-jobs` is not being setup correctly in core. As I'm trying to reproduce the issue, I notice that some elements which don't have `notparallel` set are being built with `-j1`. I'm not sure what's happening exactly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
gtristan commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-1017475406
> I think that's indeed the right thing to do here as far as I can tell
I think this is clearly wrong.
Any code that I can think of which composites one mapping on top of another mapping, expects the source mapping to remain unmodified by the operation, only the target mapping should be affected by composition.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan edited a comment on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
gtristan edited a comment on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-1017475406
> I think that's indeed the right thing to do here as far as I can tell
I think this is clearly wrong.
Any code that I can think of which composites one mapping on top of another mapping, expects the source mapping to remain unmodified by the operation, only the target mapping should be affected by composition.
What is likely happening here is that `MappingNode._composite()` is failing to overwrite the target mapping with clones of the source node, which it should be doing for mutable values at least.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] BenjaminSchubert commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
BenjaminSchubert commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-1016798246
I think that's indeed the right thing to do here as far as I can tell
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] jjardon commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
jjardon commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-1014799874
Build in fdsdk using bst2 fail because of this atm: https://gitlab.com/freedesktop-sdk/freedesktop-sdk/-/jobs/1979958539
@gtristan
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
gtristan commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-1015005516
For now I'm adding this basic test which shows notparallel working properly: https://github.com/apache/buildstream/pull/1570
It would be good to expand on this to reproduce this quirky behavior we are seeing in fdsdk.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] nanonyme commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
nanonyme commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-1020860097
Maybe composite itself could be unittested to get it to break?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
abderrahim commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-1020845732
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
abderrahim commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-986940137
What I have noticed is that if the element I'm passing on the command line has `notparallel` set, other elements are built with `-j1` and if it doesn't have `notparallel` set, elements with `notparallel` are built with `-j8`.
My current theory is that the variable in the plugin yaml are being expanded on the first use of the plugin (rather than making a copy and expanding the variables there).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] nanonyme commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
nanonyme commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-1020174929
Are we still going to merge the bug fix? This is from freedesktop-sdk point of view beta blocker.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan edited a comment on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
gtristan edited a comment on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-986390997
> I can reproduce this issue, can someone reopen this or should I file a new one?
This feels unlikely, with which plugin are you able to reproduce? It is the plugin YAML which is responsible for setting `MAKEFLAGS` according to `%{max-jobs}` (but that is already mentioned in Chandan’s comment).
Is `%{max-jobs}` not being setup properly in the core ?
Or, is it possible you are confusing the role of `notparallel` with the role of the `—builders` cli option ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan closed issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
gtristan closed issue #1360:
URL: https://github.com/apache/buildstream/issues/1360
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
abderrahim commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-1020845732
Nope, my failed attempt is here: https://github.com/abderrahim/incorrect-max-jobs
I'll try to find out what the problem is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
gtristan commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-1020781095
[...]
> Here is a minimal reproducer for the issue:
>
> ```python
> from buildstream.node import Node
> from buildstream._variables import Variables
>
> default = Node.from_dict({'MAKEFLAGS': '-j%{max-jobs}'})
> element = Node.from_dict({'name': 'my-element.bst'})
> v = Variables(Node.from_dict({'max-jobs': '1'}))
>
> print(default, element, sep='\n')
> # [synthetic node]: {'MAKEFLAGS': '-j%{max-jobs}'}
> # [synthetic node]: {'name': 'my-element.bst'}
>
> default._composite(element)
>
> print(default, element, sep='\n')
> # [synthetic node]: {'MAKEFLAGS': '-j%{max-jobs}'}
> # [synthetic node]: {'name': 'my-element.bst', 'MAKEFLAGS': '-j%{max-jobs}'}
>
> v.expand(element)
>
> print(default, element, sep='\n')
> # [synthetic node]: {'MAKEFLAGS': '-j1'}
> # [synthetic node]: {'name': 'my-element.bst', 'MAKEFLAGS': '-j1'}
> ```
So I'm trying to put together a minimal reproducer with a proper end-to-end test in `tests/format/variables.py` but still having trouble reproducing this.
I've got 2 manual elements where one depends on the other, and I'm unable to get one of them to pollute the other, by setting `notparallel` in one element and observing `max-jobs` in `bst show --format '%{vars}'` output.
Setting either element to `notparallel` does not pollute the second element, even adding a third element where the base element and toplevel element are both `notparallel` but the middle element does not specify `notparallel` still yields the expected result.
Were you able to reproduce this in a simplified end to end test ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
gtristan commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-1028738413
Now #1570 has been updated to contain both:
* The updated test case which triggers the issue
* The fix for `MappingNode._composite()` which ensures that we overwrite target nodes with *clones* of source nodes, so that the composite source is not left vulnerable to mutations when code goes on to mutate the composited *target* node.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan edited a comment on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
gtristan edited a comment on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-986390997
> I can reproduce this issue, can someone reopen this or should I file a new one?
This feels unlikely, with which plugin are you able to reproduce? It is the plugin YAML which is responsible for setting `MAKEFLAGS` according to `%{max-jobs}` (but that is already mentioned in Chandan’s comment).
Is `%{max-jobs}` not being setup properly in the core ?
Or, is it possible you are confusing the role of `notparallel` with the role of the `—builders` cli option ?
Or, is it possible that `-j` is being specified somewhere else at a higher priority? E.g. explicit setting in the `.bst` file environment settings or explicitly in the `build-commands` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
abderrahim commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-986495486
I'm still not sure exactly what happens as I'm yet to reproduce it locally but here is the element: https://gitlab.com/freedesktop-sdk/freedesktop-sdk/-/blob/abderrahim/bst2/elements/components/slang.bst
And here is a failing build https://gitlab.com/freedesktop-sdk/freedesktop-sdk/-/jobs/1849722318/artifacts/file/cache/buildstream/logs/freedesktop-sdk/components-slang/9cfbabfb-build.169.log (notice MAKEFLAGS is set to -j8)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan edited a comment on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
gtristan edited a comment on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-986390997
> I can reproduce this issue, can someone reopen this or should I file a new one?
This feels unlikely, with which plugin are you able to reproduce? It is the plugin YAML which is responsible for setting `MAKEFLAGS` according to `%{max-jobs}` (but that is already mentioned in another comment).
Is `%{max-jobs}` not being setup properly in the core ?
Or, is it possible you are confusing the role or `notparallel` with the role of the `—builders` cli option ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
gtristan commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-986390997
> I can reproduce this issue, can someone reopen this or should I file a new one?
This feels unlikely, with which plugin are you able to reproduce? It is the plugin YAML which is responsible for setting `MAKEFLAGS` according to `%{max-jobs}` (but that is already mentioned here).
Is `%{max-jobs}` not being setup properly in the core ?
Or, is it possible you are confusing the role or `notparallel` with the role of the `—builders` cli option ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
abderrahim commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-986264649
I can reproduce this issue, can someone reopen this or should I file a new one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
gtristan commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-1020196672
> Are we still going to merge the bug fix? This is from freedesktop-sdk point of view beta blocker.
I don’t want to merge the workaround for what I think is clear cut broken behavior of `MappingNode._composite()`, this is probably an indicator of further brokenness we haven’t discovered yet. Probably this went unnoticed until now because we don’t do very much compositing of nested dictionaries and lists.
I’ve asked @BenjaminSchubert to come back and comment on this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan commented on issue #1360: `notparallel` variable is broken
Posted by GitBox <gi...@apache.org>.
gtristan commented on issue #1360:
URL: https://github.com/apache/buildstream/issues/1360#issuecomment-1015019700
Maybe the lines of code in master have shifted since this comment, my 2834 says:
```python
self.__environment = unexpanded_env.strip_node_info()
```
> So the above theory is correct: expanding the variables in https://github.com/apache/buildstream/blob/master/src/buildstream/element.py#L2834 changes the value of the class variable `__defaults`.
I'll try to summarize what *should* be happening here in `__initialize_from_yaml()` (which is outlined [here in the docs](https://docs.buildstream.build/master/format_intro.html#composition):
* `self.__init_defaults()` will initialize the **class** variable `__defaults`, this is a composition of project level overrides on top of plugin provided defaults
* Questionably this composition should also be composited on top of `project.base_variables`, I think the reason why we cannot immediately cache `builtin -> project.conf -> plugin -> project.conf overrides` is because of the dual-pass loading of `project.conf` where the first pass is not a complete load.
* `self.__extract_variables()` will compose the variables
* The previously cached `default_vars` is loaded from the **class** variable `__defaults`
* The `default_vars` (which is only plugin + overrides) is composited *on top* of the `project.conf` vars
* The `element_vars` is composited on the very top of this composition and has the *last word*
* Now that we have a fully sane composition of the variables `MappingNode` for this particular element instance, the `Variables()` object instance is created for this element instance
* This is where the `notparallel` magic happens, forcing the `max-jobs` variable to `1`, this seems like the right time to do this to me.
* `self.__extract_environment()` will now compose the environment following similar rules
* The previously cached `default_env` is loaded from the **class** variable `__defaults`
* The `default_env` (which is only plugin + overrides) is composited *on top* of the `project.conf` environment
* The `element_env` is composited on the very top of this composition and has the *last word*
* Now the previously created `Variables()` instance is used to expand variables in the fully composited `unexpanded_env`, at this point the `Variables()` instance should have `max-jobs` set to `1` already and we should have the correct `MAKEFLAGS`
> The line responsible for this is `default_env._composite(environment)` as defined in [`Element.__extract_environment()`](https://github.com/apache/buildstream/blob/0df1bfc4e27055a7974bafb40461208bf4283f26/src/buildstream/element.py#L2941). The following change fixes the issue, though I'm not sure if it's the right approach or if the `_composite()` method should be modified instead.
If your patch does indeed change something, then I think you've indeed found a bug in `MappingNode.composite()`, this function should modify the *target* node only and should not cause residual side effects in the *source* node.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org