[GitHub] [buildstream] gtristan edited a comment on issue #1360: `notparallel` variable is broken

[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