Skip to content
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

scriptelement.py: use stage_artifact() instead of stage_dependency_artifact() #1429

Open
wants to merge 1 commit into
base: bst-1
Choose a base branch
from

Conversation

Cynical-Optimist
Copy link

See original merge request on GitLab
In GitLab by [Gitlab user @abderrahimk] on Sep 22, 2020, 14:01

Description

stage_dependency_artifacts() assumes the element in question is the element
to be built and does some additonal things when it is workspaced.

This fixes failure when an element that is used in the layout of a "script" element is workspaced and one of the dependencies of the last successful build is no longer in the cache.


…tifacts()

stage_dependency_artifacts() assumes the element in question is the element
to be built and does some additonal things when it is workspaced.
@Cynical-Optimist
Copy link
Author

In GitLab by [Gitlab user @abderrahimk] on Sep 22, 2020, 14:02

added 1 commit

  • dae098c - scriptelement.py: use stage_artifact() instead of stage_dependency_artifacts()

Compare with previous version

@Cynical-Optimist
Copy link
Author

In GitLab by [Gitlab user @abderrahimk] on Sep 22, 2020, 14:44

changed the description

@Cynical-Optimist
Copy link
Author

In GitLab by [Gitlab user @tristanvb] on Sep 22, 2020, 16:09

Note that before very recently (this weekend), multiple calls to either Element.stage_artifact() or Element.stage_dependency_artifacts() have a bad side effect of failing to report overlap warnings.

What's more, overlap whitelists cannot work across multiple calls.

Another drawback of the original ScriptElement I noticed while porting it, is that the layout internally is stored as a list, and not a dictionary. That means ScriptElement stages multiple dependencies into the same staging location separately, instead of staging the common dependencies for the same sandbox location at once.

This means that the staging order is not what one might expect, common dependencies written to the same target location later on in the layout will overwrite the same dependencies written before (which will also have side effects when intentionally overlapping files).

The same goes for integration commands, if multiple elements are staged into / separately, the order of integration commands being run may conflict, and will almost certainly run redundant integration commands for common dependencies.

It might be possible to at least address parts of this by:

  • Backporting OverlapCollector
  • Reporting warnings which were previously (erronously) ignored
  • Internally collecting the common dependencies targetting the same location and staging / running integration commands once for each target location (while preserving the current order dependent layout list for the target locations, which is bst-1 API)

Since scriptelement is a core element, it could use some hidden facets of the Element.dependencies() and related APIs in order to stage/run integration commands on common dependencies of a list of elements (rather than being limited to iterating over one element's dependencies at a time).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants