-
Notifications
You must be signed in to change notification settings - Fork 372
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
Redesign Win32 build transition rules #1109
Open
yukawa
wants to merge
1
commit into
google:master
Choose a base branch
from
yukawa:issue_1108
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
matsuza
reviewed
Nov 19, 2024
matsuza
reviewed
Nov 19, 2024
matsuza
reviewed
Nov 19, 2024
yukawa
force-pushed
the
issue_1108
branch
2 times, most recently
from
November 19, 2024 07:45
add042e
to
99f8fe1
Compare
Addressed all the comments then rebased onto |
yukawa
commented
Dec 5, 2024
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.
Addressed all the comments.
This reworks my previous commits [1][2][3], which aimed to make it configurable on which executable should be built with what build configurations (e.g. CRT link type, target CPU architecture). One thing we need to further consider is the debug symbol name embedded in each artifact executable [4]. To generate debug symbols one can use 'generate_pdb_file' feature. The issue is that the pdb filename is not configurable in 'rules_cc'. For instance, if the target name is 'mozc_tip32', then 'rules_cc' assumes that the output pdb file is always 'mozc_tip32.pdb'. To make it 'mozc_tip32.dll.pdb', the target name must be 'mozc_tip32.dll.dll'. This is why I had do rework Win32 build transition rules. Implementation note: * The reason why 'mozc_win32_cc_prod_binary' needs to be introduced is because the final executable name needs to be available as an input of 'mozc_cc_binary'. * 'mozc_cc_win32_library' also needs to be reworked so that 'generate_pdb_file' feature will not be applied to it. There must be no observable change in GYP build and non-Windows bazel builds. Closes google#1108. [1]: 5efa371 [2]: ff64988 [3]: ea55af0 [4]: 0377ddd
Also squashed then rebased onto HEAD. |
hiroyuki-komatsu
added a commit
that referenced
this pull request
Dec 9, 2024
* #1109 This change is a preparation to merge PR#1109. The difference from PR#1109 is the implementation of `mozc_win32_cc_prod_binary` and `mozc_cc_win32_library`. * mozc_win32_cc_prod_binary: In this change, this is a wrapper of `mozc_win_build_target` migrated from `trasition.bzl` to `build_defs.bzl`. * mozc_cc_win32_library: In this change, nothing is changed. PiperOrigin-RevId: 704201709
We have submitted bc546b2 that is a part of this PR. Could you please rebase this PR? |
hiroyuki-komatsu
added a commit
to hiroyuki-komatsu/mozc
that referenced
this pull request
Dec 18, 2024
* Follow-up to cl/704201709 and PR#1109. * google#1109
hiroyuki-komatsu
added a commit
that referenced
this pull request
Dec 19, 2024
* Follow-up to cl/704201709 and PR#1109. * #1109 #codehealth PiperOrigin-RevId: 707448701
yukawa
added a commit
to yukawa/mozc
that referenced
this pull request
Dec 23, 2024
This follows up to our previous commit [1] that was to simplicy google#1109 but accidentally removed 'linkshared' from Mozc's TIP DLL targets. While a subsequent commit [2] addressed the immediate issue by passing 'static_crt' to 'linkshared', strictly speaking they are two orthogonal concepts. Let's decouple them to avoid future confusions. There must be no immediate change in the final artifacts with this commit right now. [1]: bc546b2 [2]: 8d20ea6
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This reworks my previous commits, which aimed to make it configurable on which executable should be built with what build configurations (e.g. CRT link type, target CPU architecture).
One thing we need to further consider is the debug symbol name embedded in each artifact executable as noted in 0377ddd.
To generate debug symbols one can use
generate_pdb_file
feature. The issue is that the pdb filename is not configurable inrules_cc
. For instance, if the target name ismozc_tip32
, thenrules_cc
assumes that the output pdb file is alwaysmozc_tip32.pdb
. To make itmozc_tip32.dll.pdb
, the target name must bemozc_tip32.dll.dll
. This is why I had do rework Win32 build transition rules.Implementation note:
mozc_win32_cc_prod_binary
needs to be introduced is because the final executable name needs to be available as an input ofmozc_cc_binary
.mozc_cc_win32_library
also needs to be reworked so thatgenerate_pdb_file
feature will not be applied to it.There must be no observable change in GYP build and non-Windows bazel builds.
Closes #1108.
Issue IDs
mozc_tip32.dll
built with Bazel ismozc_tip.pdb
rather thanmozc_tip32.dll.pdb
#1108Steps to test new behaviors (if any)
bazel --bazelrc=windows.bazelrc build --config oss_windows --config release_build //win32/tip:mozc_tip32
cp bazel-bin\win32\tip\mozc_tip32.dll .
dumpbin /headers mozc_tip32.dll | findstr cv
mozc_tip32.dll.pdb
is in the output.