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 emscripten CI after recent emsdk change #11293

Merged
merged 1 commit into from
May 29, 2020
Merged

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented May 29, 2020

See emscripten-core/emsdk#472

Turns out we were actually depending on the non-embedded
config!

See emscripten-core/emsdk#472

Turns out we were actually depending on the non-embedded
config!
@sbc100 sbc100 requested a review from kripken May 29, 2020 17:52
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this worries me that more users will break then...

I assume the plan is to switch CI to embedded, but it would take more work than makes sense to do right now to unbreak? (how much work is that?)

@sbc100
Copy link
Collaborator Author

sbc100 commented May 29, 2020

I agree there could other unexpected fallout. That is why I will leave the --no-embedded option in there for a while until we can figure out all the use cases.

Our use case is pretty weird here. We use emsdk here but never call ./emsdk_env.sh which means none of the emsdk environment variables get set... and then we expect to be able to use our own version of emscripten it. But I think we can figure out way to switch to the embedded config here anyway.

@sbc100
Copy link
Collaborator Author

sbc100 commented May 29, 2020

Yes.. exactly .. unbreak first.. then look at making the switch.

@sbc100 sbc100 merged commit 119c0d1 into master May 29, 2020
@sbc100 sbc100 deleted the fix_ci_after_emsdk_change branch May 29, 2020 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants