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

Add pkg_install(wipe_destdir: bool) #893

Closed
jacky8hyf opened this issue Sep 19, 2024 · 2 comments · Fixed by #894
Closed

Add pkg_install(wipe_destdir: bool) #893

jacky8hyf opened this issue Sep 19, 2024 · 2 comments · Fixed by #894
Labels
P2 An issue that should be worked on when time is available

Comments

@jacky8hyf
Copy link
Contributor

I want to request a feature to add the following:

pkg_install(
    wipe_destdir = True,
)

Default value is false. If true, pkg_install() script will wipe the given destdir before installing the files.

This is particularly useful if we want the resulting destdir to be always strictly containing the given list of files, but not any old files. For example, I have a pkg_install() rule that generates stardoc and put it in our source tree. If the list of generated file names changes, I need to manually delete the old documentation files now. But with this attribute, I can set it to True so I don't have to manually delete old documentation. The example is illustrated here: https://r.android.com/c/kernel/build/+/3273727/2/kleaf/docs/BUILD.bazel

@aiuto if you think this is a useful feature, I can start preparing a pull request; or please let me know if this does not fit in pkg_install(). Thanks!

@aiuto
Copy link
Collaborator

aiuto commented Sep 20, 2024

Sigh... Yes it is useful. but adding an rm -rf is such a footgun.
What if is were a command line option for when you run it?
blaze run //my/install:target -- --rules_pkg//pkg:install-wipe-dest.

@aiuto aiuto added the P2 An issue that should be worked on when time is available label Sep 20, 2024
@jacky8hyf
Copy link
Contributor Author

jacky8hyf commented Sep 20, 2024

That would be sub-optimal for our use case (and for some downstream partners), so I would love to add an attribute to "always wipe". But yeah it is better than nothing.

I'll submit a PR to add the command line option (the option goes to the script, not to bazel, so it goes after the -- and doesn't need a @rules_pkg//pkg:install prefix.

bazel run //my/install:target -- --wipe-destdir

Bikeshed:

How do you like the command line option name to be? wipe_destdir, wipe-destdir, wipe-dest, wipe_dest etc.? Personally I like wipe_destdir but there are no convention in this code base, so I'd like to ask first.

jacky8hyf pushed a commit to jacky8hyf/rules_pkg that referenced this issue Sep 20, 2024
If specified, wipe destination directory before installing.

Fixes bazelbuild#893.
aiuto pushed a commit that referenced this issue Sep 28, 2024
* install: test uses pathlib.

Cleans up a lot of code!

* install: add --wipe_destdir option.

If specified, wipe destination directory before installing.

Fixes #893.

* install: Update doc for --wipe_destdir.

Clarify that this will delete the whole directory.

---------

Co-authored-by: HONG Yifan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 An issue that should be worked on when time is available
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants