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

Fix wasm dynamic library extension crash #17698

Closed

Conversation

zaucy
Copy link
Contributor

@zaucy zaucy commented Mar 8, 2023

This is an amendment to #17374

We have a C++ toolchain config that's being developed to support standalone wasm. We discovered that #17374 wasn't complete. Our example was poorly written and didn't actually create a wasm dynamic library. These changes allow us to successfully create a standalone wasm dynamic library. Sorry for the botched attempt previously.

I would like to add tests, but I'm unsure how to approach such a tests considering the automatic toolchain doesn't support wasm AFAICT.

@zaucy zaucy requested a review from lberki as a code owner March 8, 2023 22:03
@zaucy
Copy link
Contributor Author

zaucy commented Mar 8, 2023

Is it possible to fit this into the upcoming 6.1.1 release? #17374 was in 6.1.0.

@zaucy zaucy requested a review from oquenchil as a code owner March 8, 2023 22:32
@ShreeM01 ShreeM01 added team-Rules-CPP Issues for C++ rules awaiting-review PR is awaiting review from an assigned reviewer labels Mar 8, 2023
@keertk
Copy link
Member

keertk commented Mar 8, 2023

Hi @zaucy I think we'll have to wait till 6.2.0 to include this. 6.1.1 is a patch release to address only regressions.
(but cc @meteorcloudy let us know if you think otherwise)

@keertk
Copy link
Member

keertk commented Mar 8, 2023

@bazel-io fork 6.2.0

@meteorcloudy
Copy link
Member

Looks like this is fixing a bug in 6.1.0, I think it's fine to also cherry pick this for 6.1.1.

/cc @oquenchil Can you help review this?

@keertk
Copy link
Member

keertk commented Mar 9, 2023

@bazel-io fork 6.1.1

@oquenchil oquenchil self-assigned this Mar 13, 2023
@meteorcloudy meteorcloudy added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Mar 13, 2023
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Mar 13, 2023
ShreeM01 pushed a commit to ShreeM01/bazel that referenced this pull request Mar 13, 2023
This is an amendment to bazelbuild#17374

We have a C++ toolchain config that's being developed to support standalone wasm. We discovered that bazelbuild#17374 wasn't complete. Our example was poorly written and didn't actually create a wasm dynamic library. These changes allow us to successfully create a standalone wasm dynamic library. Sorry for the botched attempt previously.

I would like to add tests, but I'm unsure how to approach such a tests considering the automatic toolchain doesn't support wasm AFAICT.

Closes bazelbuild#17698.

PiperOrigin-RevId: 516204125
Change-Id: Iced5cc80a3151ffde7116b6264c89eaf40466ff5
ShreeM01 added a commit that referenced this pull request Mar 13, 2023
This is an amendment to #17374

We have a C++ toolchain config that's being developed to support standalone wasm. We discovered that #17374 wasn't complete. Our example was poorly written and didn't actually create a wasm dynamic library. These changes allow us to successfully create a standalone wasm dynamic library. Sorry for the botched attempt previously.

I would like to add tests, but I'm unsure how to approach such a tests considering the automatic toolchain doesn't support wasm AFAICT.

Closes #17698.

PiperOrigin-RevId: 516204125
Change-Id: Iced5cc80a3151ffde7116b6264c89eaf40466ff5

Co-authored-by: Ezekiel Warren <[email protected]>
ShreeM01 pushed a commit to ShreeM01/bazel that referenced this pull request Mar 13, 2023
This is an amendment to bazelbuild#17374

We have a C++ toolchain config that's being developed to support standalone wasm. We discovered that bazelbuild#17374 wasn't complete. Our example was poorly written and didn't actually create a wasm dynamic library. These changes allow us to successfully create a standalone wasm dynamic library. Sorry for the botched attempt previously.

I would like to add tests, but I'm unsure how to approach such a tests considering the automatic toolchain doesn't support wasm AFAICT.

Closes bazelbuild#17698.

PiperOrigin-RevId: 516204125
Change-Id: Iced5cc80a3151ffde7116b6264c89eaf40466ff5
ShreeM01 added a commit that referenced this pull request Mar 14, 2023
This is an amendment to #17374

We have a C++ toolchain config that's being developed to support standalone wasm. We discovered that #17374 wasn't complete. Our example was poorly written and didn't actually create a wasm dynamic library. These changes allow us to successfully create a standalone wasm dynamic library. Sorry for the botched attempt previously.

I would like to add tests, but I'm unsure how to approach such a tests considering the automatic toolchain doesn't support wasm AFAICT.

Closes #17698.

PiperOrigin-RevId: 516204125
Change-Id: Iced5cc80a3151ffde7116b6264c89eaf40466ff5

Co-authored-by: Ezekiel Warren <[email protected]>
fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
This is an amendment to bazelbuild#17374

We have a C++ toolchain config that's being developed to support standalone wasm. We discovered that bazelbuild#17374 wasn't complete. Our example was poorly written and didn't actually create a wasm dynamic library. These changes allow us to successfully create a standalone wasm dynamic library. Sorry for the botched attempt previously.

I would like to add tests, but I'm unsure how to approach such a tests considering the automatic toolchain doesn't support wasm AFAICT.

Closes bazelbuild#17698.

PiperOrigin-RevId: 516204125
Change-Id: Iced5cc80a3151ffde7116b6264c89eaf40466ff5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants