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

feat(command): add support for creates and consumes properties for co… #505

Merged
merged 20 commits into from
Sep 21, 2022

Conversation

simbuerg
Copy link
Member

…mmands

This adds support for new command properties.

creates:
You can track all artifacts that will be created by this command using a Path rendered from a PathToken.

consumes:
You can track all artifacts that will be consumed by a command using a Path rendered from a PathToken.

With creates and consumes defined, we provide a contextmanager that enables customizable rollback functionality for project commands.

Example:

with command.enable_rollback(project_command):
	project_command(...)

This will perform a backup of all "consumed" artifacts, and restore the backup after the run. Furthermore, all created artifacts will be deleted. You can customize this behavior, by providing custom Callables for the 3 stages:
- backup
- restore
- prune

…mmands

This adds support for new command properties.

creates:
You can track all artifacts that will be created by this command using
a Path rendered from a PathToken.

consumes:
You can track all artifacts that will be consumed by a command using a Path
rendered from a PathToken.

With creates and consumes defined, we provide a contextmanager that enables
customizable rollback functionality for project commands.

Example:
```
with command.enable_rollback(project_command):
	project_command(...)
```

This will perform a backup of all "consumed" artifacts, and restore the backup
after the run. Furthermore, all created artifacts will be deleted.
You can customize this behavior, by providing custom Callables for the 3 stages:
	- backup
	- restore
	- prune
@codecov
Copy link

codecov bot commented Sep 14, 2022

Codecov Report

Base: 50.89% // Head: 50.74% // Decreases project coverage by -0.14% ⚠️

Coverage data is based on head (537a80e) compared to base (761ee37).
Patch coverage: 41.86% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #505      +/-   ##
==========================================
- Coverage   50.89%   50.74%   -0.15%     
==========================================
  Files         124      124              
  Lines        8107     8207     +100     
  Branches     1254     1274      +20     
==========================================
+ Hits         4126     4165      +39     
- Misses       3822     3882      +60     
- Partials      159      160       +1     
Impacted Files Coverage Δ
benchbuild/command.py 47.75% <39.51%> (-3.61%) ⬇️
benchbuild/utils/actions.py 55.71% <100.00%> (+0.08%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@simbuerg simbuerg requested a review from vulder September 14, 2022 21:30
project = project_command.project

for created in command.creates:
created_path = created.render(project=project)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add a few sanity checks here. I'm always uneasy when using rm with a renderer.

created_path != '/'
created_path != '/home/'
created_path != '/home/user/'

Hope you get my point 😆

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I'm in the same boat.
Imho it should even be restricted to project.builddir.

Comment on lines 391 to 392
creates: tp.Optional[tp.List[PathToken]] = None,
consumes: tp.Optional[tp.List[PathToken]] = None,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if only PathToken gives a easy to user interface here. If I did not miss something I need to always create a const string token here? Would be taking Union[PathToken, str] be ok and convert the str automatically?

Maybe this is not correct and we do have to prefix every path with the source root?
However, this would introduce quite a lot of duplication.

Copy link
Member Author

@simbuerg simbuerg Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ConstStr should not be necessary. I would expect the PathToken to begin with a SourceRoot or ProjectRoot most of the time. If you provide a str, what should the default behavior be? I would be ok with "default to ProjectRoot."

@simbuerg simbuerg merged commit 89ca4fc into master Sep 21, 2022
@simbuerg simbuerg deleted the simbuerg/command-cleanup branch September 21, 2022 19:44
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