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

emscripten 2.0.10 #66150

Closed
wants to merge 1 commit into from
Closed

Conversation

carlocab
Copy link
Member

@carlocab carlocab commented Dec 3, 2020

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

@BrewTestBot BrewTestBot added nodejs Node or npm use is a significant feature of the PR or issue python Python use is a significant feature of the PR or issue labels Dec 3, 2020
@alebcay
Copy link
Member

alebcay commented Dec 3, 2020

I was holding off on this bump for a while because there was no associated binaryen version bump with it, but frankly, if it works, I'd be okay with crossing my fingers and shipping it.

If emscripten and binaryen end up being so closely coupled (without tagged releases from one or the other), it might make sense to just have binaryen as a resource for emscripten (at the specified commit) while having a binaryen formula as well. The approach irks me a bit (imagine having two copies of binaryen lying around) but the real solution would just be for the respective upstream teams to coordinate releases better, I think.

@carlocab
Copy link
Member Author

carlocab commented Dec 3, 2020

Two copies of binaryen? I have two copies of llvm! (Arguably three, because the Xcode toolchain is practically a whole other copy. You can now see why I was so keen on cutting down the bundled llvm.)

Bundling binaryen is perfectly fine though: it's only 35MB.

Though, what I don't understand is: why can't emscripten do their releases based on released versions of their dependencies??

@carlocab
Copy link
Member Author

carlocab commented Dec 3, 2020

Nope, this doesn't work. Any chance you know what the problem is, @alebcay? Hopefully, it's just that we're not tracking their preferred version of binaryen...

@alebcay
Copy link
Member

alebcay commented Dec 3, 2020

I don't really know what the issue is here. I think the error being thrown is from https://github.com/WebAssembly/binaryen/blob/00f96c854ed3691b01fa35bba0a1d010d08958bd/src/tools/wasm-emscripten-finalize.cpp#L312.

@carlocab
Copy link
Member Author

carlocab commented Dec 3, 2020

Yep, I agree. Unfortunately, I don't find the error or surrounding code informative about what the problem is...

@carlocab
Copy link
Member Author

carlocab commented Dec 4, 2020

I guess there are two possible ways forward with this PR:

  1. Bundle emscripten's preferred version of binaryen as a resource.
  2. Try emscripten 2.0.8 (it was 2.0.7 before this bump), maybe that plays better with Homebrew binaryen.

The two options are not mutually exclusive, but it would be silly to do them both. Do you have any other ideas or a preference between either of those, @alebcay?

@carlocab
Copy link
Member Author

carlocab commented Dec 4, 2020

Decided to go with option 1 for now, though I am happy to change this with feedback.

@carlocab
Copy link
Member Author

carlocab commented Dec 4, 2020

I am wondering about the license though.

Since we are packaging emscripten with llvm (and, if this PR works, with binaryen), shouldn't this formula technically also be bound by the licenses of llvm (Apache-2.0 + llvm exceptions) and binaryen (also Apache-2.0)?

@SMillerDev
Copy link
Member

Since we are packaging emscripten with llvm (and, if this PR works, with binaryen), shouldn't this formula technically also be bound by the licenses of llvm (Apache-2.0 + llvm exceptions) and binaryen (also Apache-2.0)?

I think it should, yes.

@carlocab
Copy link
Member Author

carlocab commented Dec 4, 2020

Ok, cool. Will make that change if the build succeeds.

@carlocab carlocab changed the title emscripten 2.0.9 emscripten 2.0.10 Dec 4, 2020
@carlocab
Copy link
Member Author

carlocab commented Dec 4, 2020

10.14 and 10.15 runners don't seem to be working. Best I can tell, everything's been queued since at least three hours ago (see all the newer PRs). Weird.

@carlocab
Copy link
Member Author

carlocab commented Dec 4, 2020

Builds on Big Sur. Still queued on Catalina and Mojave. Guess Homebrew binaryen wasn't new enough.

- bumped emscripten version
- changed llvm revision according to (see formula for details):
  - https://github.com/emscripten-core/emsdk/blob/master/emscripten-releases-tags.txt
  - https://chromium.googlesource.com/emscripten-releases/+/37fc7647c754ac9a28ad588c143b82286de0ef71/DEPS
- use recommended binaryen revision
    - The same has been done with llvm. See: Homebrew#63183
- updated license to reflect bundled llvm and binaryen
@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

@carlocab carlocab deleted the emscripten-2.0.9 branch December 7, 2020 17:18
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Jan 7, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Jan 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
nodejs Node or npm use is a significant feature of the PR or issue outdated PR was locked due to age python Python use is a significant feature of the PR or issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants