-
Notifications
You must be signed in to change notification settings - Fork 21
Update Rules_Py & Consolidate internal structure #378
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
Update Rules_Py & Consolidate internal structure #378
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run //src:license-checkStatus: Click to expand output |
src/helper_lib/__init__.py
Outdated
| # ) | ||
|
|
||
|
|
||
| def get_runfiles_dir(start_path: Path | None = None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the function get_runfiles_dir defined and implemented in two different files ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be the case.
I have made a new commit where the old commented out one is deleted. Thanks for seeing that.
9441a7c to
6aaa96c
Compare
|
@MaximilianSoerenPollak please update/rebase/merge, we should get more reasonable test feedback then due to other merged PRs |
Change all 'os.getenv[RUNFILES]' to now use the find_runfiles file. This is in preperation for the rules_python upgrade which changes this implementation again.
6aaa96c to
735a8fd
Compare
| json_file_raw = f"{e.bazel_module}+/{e.target}/_build/needs/needs.json" | ||
| r = get_runfiles_dir() | ||
| json_file = r / json_file_raw | ||
| logger.debug(f"Fixed JSON file path: {json_file_raw} -> {json_file}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| json_file_raw = f"{e.bazel_module}+/{e.target}/_build/needs/needs.json" | |
| r = get_runfiles_dir() | |
| json_file = r / json_file_raw | |
| logger.debug(f"Fixed JSON file path: {json_file_raw} -> {json_file}") | |
| json_file = get_runfiles_dir() / f"{e.bazel_module}+" / e.target / "_build/needs/needs.json" | |
| logger.debug(f"External needs.json: {json_file}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in newest commit.
| else: | ||
| r = get_runfiles_dir() | ||
| if "ide_support.runfiles" in str(r): | ||
| logger.error("Combo builds are currently only supported with Bazel.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to add reference integration to consumer tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should yes.
|
The created documentation from the pull request is available at: docu-html |
| os.environ["RUNFILES_DIR"] = str(runfiles_dir) | ||
| with pytest.raises(SystemExit) as e: | ||
| get_runfiles_dir() | ||
| assert "Could not find runfiles_dir" in str(e.value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh thanks! I did now know I can keep the e for further asserts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR upgrades Bazel Python tooling (rules_python / score_tooling) and consolidates Bazel runfiles discovery by removing find_runfiles and moving the needed logic into helper_lib.
Changes:
- Upgraded Bazel module deps (
rules_pythonandscore_tooling) and refreshed parts of the Python lockfile. - Removed
src/find_runfilesand rewired extensions to usesrc.helper_lib.get_runfiles_dir. - Added new unit tests covering runfiles directory resolution behavior.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
MODULE.bazel |
Upgrades rules_python and score_tooling to enable newer rules_python usage. |
src/requirements.in |
Adjusts Sphinx version constraint and minor formatting. |
src/requirements.txt |
Updates several pinned packages/hashes as part of dependency refresh. |
src/helper_lib/__init__.py |
Adds centralized get_runfiles_dir() using python.runfiles.Runfiles. |
src/helper_lib/test_helper_lib.py |
Adds tests for runfiles-dir discovery and fallback behavior. |
src/helper_lib/BUILD |
Adds @rules_python//python/runfiles dependency for the new helper logic. |
src/extensions/score_plantuml.py |
Switches PlantUML extension to use helper_lib.get_runfiles_dir. |
src/extensions/BUILD |
Adds helper_lib as a dep for the PlantUML extension target. |
src/extensions/score_metamodel/external_needs.py |
Switches external-needs path logic to use helper_lib.get_runfiles_dir. |
src/extensions/score_metamodel/BUILD |
Ensures helper_lib is available to the metamodel extension target. |
src/extensions/score_sphinx_bundle/BUILD |
Removes dependency on deleted find_runfiles, keeps helper_lib. |
src/BUILD |
Removes find_runfiles sources from aggregation; leaves a stale commented dep. |
src/find_runfiles/__init__.py |
Deleted (logic migrated to helper_lib). |
src/find_runfiles/test_find_runfiles.py |
Deleted (tests replaced by helper_lib tests). |
src/find_runfiles/BUILD |
Deleted (package removed). |
b0b3bc8
into
eclipse-score:main
📌 Description
This PR achieves two main things.
The solution to the upgrade of the Runfiles was based on the work from @NEOatNHNG and his commit here
And also done with the help of @PiotrKorkus
🚨 Impact Analysis
✅ Checklist