-
Notifications
You must be signed in to change notification settings - Fork 1
Beautiful #2
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
base: main2
Are you sure you want to change the base?
Conversation
do not mess with mac os
let us see if it works Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
should work
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.
Hello @Kreijstal, 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!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request titled "Beautiful". Based on the generic title and description, the specific intent isn't immediately clear. However, by inspecting the changes, it appears this PR focuses on improving the node-gyp build process, particularly for Windows environments using different toolchains like MSYS2/MinGW alongside the standard MSVC. The changes involve conditional library linking in addon.gypi and significant updates to the make.py generator to handle dependency files, path normalization, object file generation, and rule writing more robustly, likely addressing compatibility issues in these varied environments.
Highlights
- Windows Toolchain Compatibility: Adds logic to
addon.gypito detect the OS type (specifically MINGW) and conditionally link different sets of libraries appropriate for MSYS2/MinGW/GCC versus MSVC builds on Windows. - Improved Dependency File Handling: Refactors the
fixup_depmake define ingyp/pylib/gyp/generator/make.pyto more robustly parse compiler-generated dependency files, handling line continuations, path normalization, and extracting dependencies correctly. - Enhanced Object File and Rule Generation: Updates the
make.pygenerator to better handle path normalization (including making absolute paths relative to the node-gyp directory), quote object file paths correctly, and generate make rules (both explicit and suffix rules) for compiling sources with improved precision. - Refined Path and Quoting Logic: Introduces more sophisticated logic in
make.py'sObjectifyandWriteListmethods to handle absolute paths, calculate relative paths within the build directory, and ensure correct quoting of paths and library names in the generated Makefiles.
Changelog
- addon.gypi
- Added
os_type%variable to detect the operating system type using Node.js (line 6). - Implemented conditional
librarieslists based onos_typestarting with 'MINGW', providing separate library sets for MSYS2/MinGW/GCC and MSVC toolchains on Windows (lines 171-205).
- Added
- gyp/pylib/gyp/generator/make.py
- Refactored the
fixup_depmake define to improve parsing of dependency files, including joining lines, normalizing slashes, extracting dependencies, and filtering empty lines (lines 443-472). - Added logic in the
Writemethod to determine the absolute path of the node-gyp directory, first from spec variables and falling back to calculating it from the script's location (lines 868-900). - Removed redundant comments in
WriteSources(lines 1438, 1447). - Significantly refactored
WriteSourcesto iterate through compilable sources, calculate and quote object paths, store mappings, generate explicit rules for absolute source paths, updateall_depsand dependency rules using quoted paths, update PCH dependencies using quoted paths, apply build flags to quoted object paths, add quoted objects and linkable sources toextra_link_deps, and generate suffix rules for sources not handled explicitly (lines 1478-1576). - Modified
WriteListto handle a special case for already-quoted library names (-l"...") to prevent double quoting (lines 2134-2144). - Refactored
Objectifyto handle absolute paths, calculate paths relative to the node-gyp directory when applicable, and apply the object directory prefix to the resulting path segment (lines 2372-2407).
- Refactored the
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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.
Build files, gyp, and make,
Paths twist, for goodness sake.
Quote them just right,
Shine a guiding light,
For builds that won't break.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
The pull request introduces changes to the build system, specifically targeting Windows and cross-compilation scenarios. The changes involve modifications to gyp files and Python scripts to handle different toolchains and library dependencies. Overall, the changes seem well-structured and address specific issues related to building Node.js addons on Windows using various toolchains. However, there are a few areas that could benefit from further clarification or improvement.
Summary of Findings
- Missing Comments: Several areas of the code lack sufficient comments, making it difficult to understand the purpose and logic behind certain decisions. Adding more comments would improve the maintainability and readability of the code.
- Complex Logic: The
fixup_depmacro inmake.pyuses a complex series ofsedcommands. Breaking this down into smaller, more manageable steps with comments would improve readability. - Path Handling: The logic for calculating and normalizing paths, especially in
make.py, could benefit from more detailed explanations to clarify the expected directory structure and the purpose of each step.
Merge Readiness
The pull request introduces important changes for Windows and cross-compilation support. While the changes appear to be well-structured, the lack of comments in certain areas makes it difficult to fully assess the impact and maintainability of the code. I recommend addressing the missing comments and clarifying the complex logic before merging. I am unable to approve this pull request, and it should be reviewed by others before merging.
gyp/pylib/gyp/generator/make.py
Outdated
| if is_in_node_gyp_dir: | ||
| try: | ||
| # Calculate path relative to abs_node_gyp_dir itself. | ||
| base_for_relpath = self.abs_node_gyp_dir # Use node_gyp_dir as base | ||
| rel_path = os.path.relpath(path, base_for_relpath) | ||
| # Ensure forward slashes for Makefile compatibility | ||
| rel_path_unix = rel_path.replace('\\', '/') | ||
| path_segment = rel_path_unix # Use the relative path segment instead! | ||
| except ValueError as e: | ||
| print(f"DEBUG [make.py Objectify]: os.path.relpath failed: {e}. Using original absolute path segment.", file=sys.stderr) |
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.
Checklist
npm install && npm run lint && npm testpassesDescription of change
see workflow