Skip to content
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
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yukawa
Copy link
Collaborator

@yukawa yukawa commented Nov 1, 2024

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 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 #1108.

Issue IDs

Steps to test new behaviors (if any)

  • OS: Windows 11 23H2
  • Steps:
    1. bazel --bazelrc=windows.bazelrc build --config oss_windows --config release_build //win32/tip:mozc_tip32
    2. cp bazel-bin\win32\tip\mozc_tip32.dll .
    3. dumpbin /headers mozc_tip32.dll | findstr cv
    4. Confirm mozc_tip32.dll.pdb is in the output.

src/build_defs.bzl Outdated Show resolved Hide resolved
src/build_defs.bzl Outdated Show resolved Hide resolved
src/build_defs.bzl Outdated Show resolved Hide resolved
@yukawa yukawa force-pushed the issue_1108 branch 2 times, most recently from add042e to 99f8fe1 Compare November 19, 2024 07:45
@yukawa
Copy link
Collaborator Author

yukawa commented Nov 19, 2024

Addressed all the comments then rebased onto HEAD.

src/build_defs.bzl Show resolved Hide resolved
src/win32/broker/BUILD.bazel Outdated Show resolved Hide resolved
src/win32/cache_service/BUILD.bazel Outdated Show resolved Hide resolved
src/win32/custom_action/BUILD.bazel Outdated Show resolved Hide resolved
src/win32/installer/BUILD.bazel Outdated Show resolved Hide resolved
src/win32/tip/BUILD.bazel Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@yukawa yukawa left a 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.

src/win32/broker/BUILD.bazel Outdated Show resolved Hide resolved
src/build_defs.bzl Show resolved Hide resolved
src/win32/cache_service/BUILD.bazel Outdated Show resolved Hide resolved
src/win32/custom_action/BUILD.bazel Outdated Show resolved Hide resolved
src/win32/installer/BUILD.bazel Outdated Show resolved Hide resolved
src/win32/tip/BUILD.bazel Outdated Show resolved Hide resolved
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
@yukawa
Copy link
Collaborator Author

yukawa commented Dec 5, 2024

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
@hiroyuki-komatsu
Copy link
Collaborator

We have submitted bc546b2 that is a part of this PR.
I hope it makes this PR focus on the essential part.

Could you please rebase this PR?
Thank you,

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
Labels
None yet
Projects
None yet
3 participants