-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Tools cleanup #2950
Tools cleanup #2950
Conversation
f0590fa
to
4cd87e8
Compare
tools/format.py
Outdated
@@ -15,6 +14,8 @@ | |||
parser.add_argument("--gn", help="only run gn format", action="store_true") | |||
parser.add_argument("--cc", help="only run clang format", action="store_true") | |||
|
|||
clang_format_path = os.path.join(third_party_path, "depot_tools", | |||
"clang-format") |
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.
Could we directly call clang_format.py
with sys.executable
here instead of the wrapper script?
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.
It is possible, but the way it was done in #2943 did not work for me (on windows).
Also, if bypassing wrapper scripts is a goal, then IMO we should just call the clang_format binary directly.
The clang_format.py wrapper creates more problems than it solves.
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, but without it I've seen strange build errors on appveyor like: https://ci.appveyor.com/project/deno/deno/builds/27388163#L329. But unfortunately I can't test it by myself on Windows, so it's quite hard to debug for me.
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.
IMHO calling the clang-format binary directly seems to be a good option for me, because otherwise after the gn root move to core/libdeno
we would need to either set CHROMIUM_BUILDTOOLS_PATH
or maintain 2 buildtools
symlinks, see: #2943 (comment)
This PR is not complete yet. I also have some updates to format.py and lint.py in the pipeline |
4cd87e8
to
b1a121d
Compare
f3553f8
to
39506ed
Compare
…nd#2950) * Remove reference to removed dir 'third_party/rust_crates'. * Remove reference to unused environment variable 'DENO_NINJA_PATH'. * Remove helper functions 'root()' and 'tp()'. * Move definition of 'third_party_path' to build.py. * Move definition of 'gn_exe()' to setup.py. * Move 'download_sccache()' and 'download_hyperfine()' from prebuilt.py to third_party.py, and delete prebuilt.py. * Add helper function 'get_platform_dir_name()' to locate the platform-specific 'v8/buildtools/<platform>' and 'prebuilt/<platform>' directories. * Add helper function 'get_prebuilt_tool_path()' that returns the full path to a platform-specific executable in //prebuilt. * Cosmetic improvements.
39506ed
to
f9b8a85
Compare
f9b8a85
to
0abe078
Compare
…nd#2950) * Remove reference to removed dir 'third_party/rust_crates'. * Remove reference to unused environment variable 'DENO_NINJA_PATH'. * Remove helper functions 'root()' and 'tp()'. * Move definition of 'third_party_path' to build.py. * Move definition of 'gn_exe()' to setup.py. * Move 'download_sccache()' and 'download_hyperfine()' from prebuilt.py to third_party.py, and delete prebuilt.py. * Add helper function 'get_platform_dir_name()' to locate the platform-specific 'v8/buildtools/<platform>' and 'prebuilt/<platform>' directories. * Add helper function 'get_prebuilt_tool_path()' that returns the full path to a platform-specific executable in //prebuilt. * Cosmetic improvements.
0abe078
to
25b4e76
Compare
25b4e76
to
4556a38
Compare
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.
LGTM
This needs to be done to make landing #2943 more manageable