Skip to content

fix(multi-versions): correctly default 'main' arg for transition rules #1316

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 20, 2023
Merged

fix(multi-versions): correctly default 'main' arg for transition rules #1316

merged 1 commit into from
Jul 20, 2023

Conversation

aignas
Copy link
Collaborator

@aignas aignas commented Jul 14, 2023 •
edited by rickeylev
Loading

This fixes a bug where the version-aware rules required main to always be explicitly
specified. This was necessary because the main file is named after the outer target
(e.g. "foo"), but usage of the main file is done by the inner target ("_foo"). The net
effect is the inner target looks for "_foo.py", while only "foo.py" is in srcs.

To fix, the wrappers set main, if it isn't already set, to their name + ".py"

Work towards #1262

@aignas aignas requested a review from rickeylev as a code owner July 14, 2023 08:37
@aignas aignas mentioned this pull request Jul 14, 2023
Closed
@aignas aignas force-pushed the fix/transition-rules-main branch 2 times, most recently from bffc8b1 to 5946109 Compare July 14, 2023 08:55
@rickeylev rickeylev changed the title fix(transitions): correctly default 'main' arg for transition rules fix(multi-versions): correctly default 'main' arg for transition rules Jul 14, 2023
@rickeylev
Copy link
Collaborator

Some fun trivia about how py_binary finds main: it's basically a suffix search. e.g.

search_for = name + ".py"
main = None
for src in srcs:
  if src.endswith(search_for):
    main = None
    break

rickeylev
rickeylev requested changes Jul 14, 2023
View reviewed changes
Copy link
Collaborator

@rickeylev rickeylev left a comment

Choose a reason for hiding this comment

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

Mostly lgtm, but one bug to fix and some nits

@aignas
fix(transitions): correctly default 'main' arg for transition rules
482c45a 
Before the change the version-aware py_binary and py_test rules would
not default the 'main' argument correctly and this change adds unit
tests and a helper function to do the defaulting.

Work towards bazel-contrib#1262
@aignas aignas force-pushed the fix/transition-rules-main branch from 5946109 to 482c45a Compare July 15, 2023 03:25
@aignas
Copy link
Collaborator Author

aignas commented Jul 15, 2023

@rickeylev, yeah, the fact that main parameter does not have to be a string ending with .py suffix also surprised me.

As for the comment about modification in place test, it is there already because the contains_exactly is used for assertions. Tested this by adding an extra kwarg (e.g. tags) into the input and the expectation started failing.

@rickeylev rickeylev added this pull request to the merge queue Jul 20, 2023
Merged via the queue into bazel-contrib:main with commit 5c5ab5b Jul 20, 2023
@aignas aignas deleted the fix/transition-rules-main branch May 13, 2024 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rickeylev rickeylev rickeylev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aignas @rickeylev