chore: better bootstrap logging#3608
Conversation
Summary of ChangesHello @rickeylev, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and debuggability of the Python bootstrap mechanism, particularly addressing cross-platform compatibility issues. It introduces more verbose logging to provide deeper insights into the bootstrap's execution flow and improves error messages for clearer problem identification. Furthermore, it includes crucial fixes for Windows environments, ensuring correct file encoding, path handling, and permissions during the build data generation and consumption processes. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces helpful improvements to the bootstrap logging and error reporting, which will aid in debugging issues during Python binary execution. The refactoring of the Windows build data writer to use .NET APIs is a robust change. However, there is a critical regression in the stage 2 bootstrap where runfiles lookup keys are incorrectly normalized with backslashes on Windows, which will break lookups. Additionally, the error handling logic in the stage 2 bootstrap should be more defensive when the runfiles lookup fails.
| rlocation_path = self.BUILD_DATA_FILE | ||
| if is_windows(): | ||
| rlocation_path = rlocation_path.replace("/", "\\") | ||
| path = runfiles.Create().Rlocation(rlocation_path) | ||
| if is_windows(): | ||
| path = os.path.normpath(path) |
There was a problem hiding this comment.
The runfiles library requires forward slashes for the lookup key (rlocation_path) on all platforms. Replacing them with backslashes on Windows will cause the lookup to fail because the manifest uses forward slashes and the library's internal logic expects them. Furthermore, os.path.normpath should only be called if the lookup returned a valid path.
| rlocation_path = self.BUILD_DATA_FILE | |
| if is_windows(): | |
| rlocation_path = rlocation_path.replace("/", "\\") | |
| path = runfiles.Create().Rlocation(rlocation_path) | |
| if is_windows(): | |
| path = os.path.normpath(path) | |
| rlocation_path = self.BUILD_DATA_FILE | |
| path = runfiles.Create().Rlocation(rlocation_path) | |
| if path and is_windows(): | |
| path = os.path.normpath(path) |
| COVERAGE_INSTRUMENTED = "%coverage_instrumented%" == "1" | ||
|
|
||
| # runfiles-root-relative path to a file with binary-specific build information | ||
| # It uses forward slashes, so must be converted for proper usage on Windows. |
There was a problem hiding this comment.
This comment is incorrect regarding the requirements of the runfiles library. The Rlocation method expects keys to be forward-slash separated, even on Windows, to match the logical paths in the runfiles manifest.
| except Exception as exc: | ||
| if hasattr(exc, "add_note"): | ||
| exc.add_note(f"runfiles lookup path: {rlocation_path}") | ||
| exc.add_note(f"exists: {os.path.exists(path)}") | ||
| can_read = os.access(path, os.R_OK) | ||
| exc.add_note(f"readable: {can_read}") | ||
| raise |
There was a problem hiding this comment.
If the runfiles lookup fails, path will be None. Calling os.path.exists(path) or os.access(path, ...) with None will raise a TypeError, which would mask the original error. The exception handling should safely check if path is not None before attempting to access the filesystem.
| except Exception as exc: | |
| if hasattr(exc, "add_note"): | |
| exc.add_note(f"runfiles lookup path: {rlocation_path}") | |
| exc.add_note(f"exists: {os.path.exists(path)}") | |
| can_read = os.access(path, os.R_OK) | |
| exc.add_note(f"readable: {can_read}") | |
| raise | |
| except Exception as exc: | |
| if hasattr(exc, "add_note"): | |
| exc.add_note(f"runfiles lookup path: {rlocation_path}") | |
| if path is not None: | |
| exc.add_note(f"resolved path: {path}") | |
| exc.add_note(f"exists: {os.path.exists(path)}") | |
| exc.add_note(f"readable: {os.access(path, os.R_OK)}") | |
| else: | |
| exc.add_note("resolved path: None (lookup failed)") | |
| raise |
rickeylev
force-pushed
the
chore.bootstrap.logging
branch
2 times, most recently
from
e364945 to
1ac813d
Compare
rickeylev
force-pushed
the
chore.bootstrap.logging
branch
from
1ac813d to
2e00f57
Compare
1 participant
Adds additional logging in the python bootstrap.