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

Try to fix MSVC Buck Build #37242

Closed
wants to merge 1 commit into from

Conversation

NickGerleman
Copy link
Contributor

@NickGerleman NickGerleman commented May 4, 2023

facebook/yoga#1269 broke building Yoga Buck targets with MSVC. The build logs suggest this is because headers may be double included from exported_headers and headers, causing redefinition. We are not saved on the MSVC build by #pragma once since headers are imported from different paths (but the Clang build seems okay with this).

This diff attempts to isolate headers so that nothing may be double exported. This is done in a little bit of a tricky way, since Starlark doesn't seem to support globs in the form of {a,b}, and Yoga's directory structure does not separate public and private headers cleanly (we should fix this, but it needs to be changed in multiple build systems at once which I want to do not in this change).

We also get a side effect that every file which isn't a public header needs to use the namespaced form to refer to a public header, even if in the same directory. Though this is going to be needed anyway if we end up wanting to move the public headers to a new directory.

Differential Revision: D45552325

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner fb-exported labels May 4, 2023
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D45552325

@analysis-bot
Copy link

analysis-bot commented May 4, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,629,448 +11
android hermes armeabi-v7a 7,941,034 +21
android hermes x86 9,115,990 +3
android hermes x86_64 8,970,294 +11
android jsc arm64-v8a 9,193,601 +12
android jsc armeabi-v7a 8,382,656 +23
android jsc x86 9,251,494 +3
android jsc x86_64 9,509,442 +13

Base commit: 523c77d
Branch: main

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D45552325

NickGerleman added a commit to NickGerleman/react-native that referenced this pull request May 4, 2023
Summary:
Pull Request resolved: facebook#37242

X-link: facebook/yoga#1278

Reviewed By: yungsters

Differential Revision: D45552325

fbshipit-source-id: 550734cfab44e41f30baf183c9ce24594fe477fd
NickGerleman added a commit to NickGerleman/yoga that referenced this pull request May 4, 2023
Summary:
X-link: facebook/react-native#37242

Pull Request resolved: facebook#1278

Reviewed By: yungsters

Differential Revision: D45552325

fbshipit-source-id: 549a9c01a45bc06b344ee4319528aadd3fc8326e
NickGerleman added a commit to NickGerleman/yoga that referenced this pull request May 4, 2023
Summary:
X-link: facebook/react-native#37242

Pull Request resolved: facebook#1278

Reviewed By: yungsters

Differential Revision: D45552325

fbshipit-source-id: 881e83b72166900fc475bcf77ec2849759766894
NickGerleman added a commit to NickGerleman/react-native that referenced this pull request May 4, 2023
Summary:
Pull Request resolved: facebook#37242

X-link: facebook/yoga#1278

Reviewed By: yungsters

Differential Revision: D45552325

fbshipit-source-id: 4393303641b7e42d1c24e0c4479870552666caba
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D45552325

NickGerleman added a commit to NickGerleman/litho that referenced this pull request May 4, 2023
Summary:
X-link: facebook/react-native#37242

X-link: facebook/yoga#1278

facebook/yoga#1269 broke building Yoga Buck targets with MSVC. The build logs suggest this is because headers may be double included from `exported_headers` and `headers`, causing redefinition. We are not saved on the MSVC build by `#pragma once` since headers are imported from different paths (but the Clang build seems okay with this).

This diff attempts to isolate headers so that nothing may be double exported. This is done in a little bit of a tricky way, since Starlark doesn't seem to support globs in the form of `{a,b}`, and Yoga's directory structure does not separate public and private headers cleanly (we should fix this, but it needs to be changed in multiple build systems at once which I want to do not in this change).

We also get a side effect that every file which isn't a public header needs to use the namespaced form to refer to a public header, even if in the same directory. Though this is going to be needed anyway if we end up wanting to move the public headers to a new directory.

Reviewed By: yungsters

Differential Revision: D45552325

fbshipit-source-id: 407fda6b062692c1016e157b066cac286c29ec49
NickGerleman added a commit to NickGerleman/yoga that referenced this pull request May 4, 2023
Summary:
X-link: facebook/react-native#37242

Pull Request resolved: facebook#1278

facebook#1269 broke building Yoga Buck targets with MSVC. The build logs suggest this is because headers may be double included from `exported_headers` and `headers`, causing redefinition. We are not saved on the MSVC build by `#pragma once` since headers are imported from different paths (but the Clang build seems okay with this).

This diff attempts to isolate headers so that nothing may be double exported. This is done in a little bit of a tricky way, since Starlark doesn't seem to support globs in the form of `{a,b}`, and Yoga's directory structure does not separate public and private headers cleanly (we should fix this, but it needs to be changed in multiple build systems at once which I want to do not in this change).

We also get a side effect that every file which isn't a public header needs to use the namespaced form to refer to a public header, even if in the same directory. Though this is going to be needed anyway if we end up wanting to move the public headers to a new directory.

Reviewed By: yungsters

Differential Revision: D45552325

fbshipit-source-id: f11852242c600044e4fb6da3aa7b522da248ca4b
NickGerleman added a commit to NickGerleman/react-native that referenced this pull request May 4, 2023
Summary:
Pull Request resolved: facebook#37242

X-link: facebook/yoga#1278

facebook/yoga#1269 broke building Yoga Buck targets with MSVC. The build logs suggest this is because headers may be double included from `exported_headers` and `headers`, causing redefinition. We are not saved on the MSVC build by `#pragma once` since headers are imported from different paths (but the Clang build seems okay with this).

This diff attempts to isolate headers so that nothing may be double exported. This is done in a little bit of a tricky way, since Starlark doesn't seem to support globs in the form of `{a,b}`, and Yoga's directory structure does not separate public and private headers cleanly (we should fix this, but it needs to be changed in multiple build systems at once which I want to do not in this change).

We also get a side effect that every file which isn't a public header needs to use the namespaced form to refer to a public header, even if in the same directory. Though this is going to be needed anyway if we end up wanting to move the public headers to a new directory.

Reviewed By: yungsters

Differential Revision: D45552325

fbshipit-source-id: e35afac1ff001057715e2039473be89c7ecb0bc8
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D45552325

NickGerleman added a commit to NickGerleman/yoga that referenced this pull request May 4, 2023
Summary:
X-link: facebook/react-native#37242

Pull Request resolved: facebook#1278

Reviewed By: yungsters

Differential Revision: D45552325

fbshipit-source-id: 9d22fa0ffa24a5994c789ada98e07070ede4b9e2
Summary:
Pull Request resolved: facebook#37242

X-link: facebook/yoga#1278

Reviewed By: yungsters

Differential Revision: D45552325

fbshipit-source-id: 723cacbebcf952776135181f9068d113da2e925d
NickGerleman added a commit to NickGerleman/litho that referenced this pull request May 4, 2023
Summary:
X-link: facebook/react-native#37242

X-link: facebook/yoga#1278

Reviewed By: yungsters

Differential Revision: D45552325

fbshipit-source-id: 3df09db5aeec30566e2d8a60105cdb59ebc7409c
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D45552325

facebook-github-bot pushed a commit to facebook/yoga that referenced this pull request May 4, 2023
Summary:
X-link: facebook/react-native#37242

Pull Request resolved: #1278

Reviewed By: yungsters

Differential Revision: D45552325

fbshipit-source-id: 5687e8ec27a7a70df66e2f89e800210e3ce21ba3
facebook-github-bot pushed a commit to facebook/litho that referenced this pull request May 4, 2023
Summary:
X-link: facebook/react-native#37242

X-link: facebook/yoga#1278

Reviewed By: yungsters

Differential Revision: D45552325

fbshipit-source-id: 5687e8ec27a7a70df66e2f89e800210e3ce21ba3
@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label May 4, 2023
@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 1cd0f57.

jeongshin pushed a commit to jeongshin/react-native that referenced this pull request May 7, 2023
Summary:
Pull Request resolved: facebook#37242

X-link: facebook/yoga#1278

Reviewed By: yungsters

Differential Revision: D45552325

fbshipit-source-id: 5687e8ec27a7a70df66e2f89e800210e3ce21ba3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged This PR has been merged. p: Facebook Partner: Facebook Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants