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

Refactor all the input processing code. #756

Merged
merged 10 commits into from
Nov 6, 2023
Merged

Refactor all the input processing code. #756

merged 10 commits into from
Nov 6, 2023

Conversation

aiuto
Copy link
Collaborator

@aiuto aiuto commented Sep 19, 2023

  • Create a MappingContext to hold behaviors and defaults
  • Change calls
    • from process_x(ctx, content_map, file_deps, <important args>, default_x=None, default_y, ...)
    • to process_x(mapping_context, <important args>)
  1. This brings the code more in line with the preferred rule writing style. Specifically, don't pass ctx around, pass the intent so you can control behavior.
  2. A major motivation for the change is adding include_runfiles to pkg_tar, where we want to sometimes have add_label_list do the runfiles inclusion for us, and other times we want to revert to legacy behavior. If we only signaled runfiles in a single attribute in ctx, that would not be possible.

Other considerations.

  • This dropped some processing of a default mode for directories being set to 0555. While looking at the code, it turns out the default is always set from pkg_mkdirs, to 0755, which is more often what people expect. In the future, we might add default_dir_mode to mapping_context. I'm not doing that today, because we should look at Package JSON manifest should support generic attributes #385 and Add tar xattr support #707 first. The whole attribute scheme is not really sufficient.
  • extensive buidifier cleanup. Creates review noise.
  • I discovered a lot of loose behavior around default modes and users/groups. Some places look improved for users/groups, but mode is done a little different. This is intentional to preserve existing behavior so the tests pass. Someone will have to go back later and determine if tightening the behavior fixes bugs or breaks existing good behavior.
  • If it was not clear from above. I preserved behavior over "correctness" so that the PR is only about code and not features.

- Create a MappingContext to hold behaviors and defaults
- Change calls
  - from `process_x(ctx, content_map, ..., default_x, default_y, ...)`
  - to `process_x(mapping_context, ...)

1. This brings the code more in line with the new preferred rule writing style.
1. A major motivation for the change is adding include_runfiles to pkg_tar, where we want
   to sometimes have add_label_list do the runfiles inclusion for us, and
   other times we want to revert to legacy behavior. If we always passed it
   in `ctx`, that would not be possible.
@aiuto aiuto marked this pull request as draft September 19, 2023 14:41
@alexeagle alexeagle removed their request for review October 20, 2023 18:15
@aiuto aiuto self-assigned this Oct 24, 2023
@aiuto aiuto added the P1 An issue that must be resolved. Must have an assignee label Oct 24, 2023
@aiuto aiuto merged commit 5664e0e into bazelbuild:main Nov 6, 2023
1 of 2 checks passed
@aiuto aiuto deleted the noctx branch November 6, 2023 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 An issue that must be resolved. Must have an assignee
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants