pkg_install: Add destdir attr & read rel paths.#886
Merged
aiuto merged 1 commit intobazelbuild:mainfrom Aug 28, 2024
Merged
Conversation
Implementation notes: Relative paths are interpreted against BUILD_WORKSPACE_DIRECTORY, not BUILD_WORKING_DIRECTORY. This is for the following reasons: The TODO tag explicitly convey the intention of using BUILD_WORKSPACE_DIRECTORY for relative paths. If destdir is specified in the attribute of the pkg_install() target, interpreting it against BUILD_WORKSPACE_DIRECTORY is much stabler. That is, no matter where your current cwd is, the destdir attribute always refers to a path relative to the workspace root. For example: ``` pkg_install(name = "my_pkg_install", destdir = "out/dest") ``` ``` cd <workspace_root>/<some_subdir> bazel run //:my_pkg_install ``` This clearly conveys that the default destdir is <workspace_root>/out/dest regardless of where the user runs the command. The cost is that the --destdir command line argument becomes trickier to understand. For example, if one is not familiar with pkg_install, and below the workspace root they run: ``` cd <workspace_root>/out bazel run //:my_pkg_install -- --destdir dest ``` They may expect the destdir to be set to <workspace_root>/out/dest; but it is in fact `<workspace_root>/dest`. We could also interpret the target attribute & the command line argument separately (e.g. pkg_install(destdir_against_workspace)), but honestly I think that's even more confusing when they interpret relative paths differently. Please let me know if this is preferred by the maintainers.
Contributor
Author
|
Also, it appears that I am making I am just making the error more user friendly. |
Contributor
Author
|
Hello, could you please take a look @aiuto ? |
aiuto
approved these changes
Aug 28, 2024
Collaborator
aiuto
left a comment
aiuto
left a comment
There was a problem hiding this comment.
I'm glad to see someone taking an interested in this slice of the code.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implementation notes:
Relative paths are interpreted against BUILD_WORKSPACE_DIRECTORY, not BUILD_WORKING_DIRECTORY. This is for the following reasons:
The TODO tag explicitly convey the intention of using BUILD_WORKSPACE_DIRECTORY for relative paths.
If destdir is specified in the attribute of the pkg_install() target, interpreting it against BUILD_WORKSPACE_DIRECTORY is much stabler. That is, no matter where your current cwd is, the destdir attribute always refers to a path relative to the workspace root. For example:
This clearly conveys that the default destdir is
<workspace_root>/out/dest regardless of where the user runs the command.
The cost is that the --destdir command line argument becomes trickier to understand. For example, if one is not familiar with pkg_install, and below the workspace root they run:
They may expect the destdir to be set to <workspace_root>/out/dest; but it is in fact
<workspace_root>/dest.We could also interpret the target attribute & the command line argument separately (e.g. pkg_install(destdir_against_workspace)), but honestly I think that's even more confusing when they interpret relative paths differently. Please let me know if this is preferred by the maintainers.