Skip to content

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

Merged
rickeylev merged 1 commit intobazel-contrib:mainfrom
aignas:fix/transition-rules-main
Jul 20, 2023
Merged

fix(multi-versions): correctly default 'main' arg for transition rules#1316
rickeylev merged 1 commit intobazel-contrib:mainfrom
aignas:fix/transition-rules-main

Conversation

@aignas
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

rickeylev commented Jul 14, 2023

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
Copy Markdown
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
Copy Markdown
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