search

emscripten-core / emsdk

Emscripten SDK
http://emscripten.org
Other
2.92k stars 662 forks source link

3.1.56 #1353

Closed aheejin closed 4 months ago

aheejin commented 4 months ago

I wonder why build-docker-image fails... Any ideas?

sbc100 commented 4 months ago

Looks like an actual emscripten bug related to https://github.com/emscripten-core/emscripten/pull/21456

kripken commented 4 months ago

Weird, I can't reproduce that error locally on ./embuilder build zlib --force. Is it relevant only when an error happens I guess?

aheejin commented 4 months ago

Is https://github.com/emscripten-core/emscripten/pull/21523 supposed fix this or is this a separate problem?

sbc100 commented 4 months ago

Is emscripten-core/emscripten#21523 supposed fix this or is this a separate problem?

Yup that is the fix.

You can update this PR or create a new one once that has rolled in.

aheejin commented 4 months ago

Thank you!

sbc100 commented 4 months ago

Hm, how did land? It looks like it did not contain https://chromium.googlesource.com/emscripten-releases/+/8385c9ea32774b0bb8d205cc329c5db38d90da31.. which means that mac builder should have failed in the same way as before.

I was expecting that you would need to create a new RC after https://chromium.googlesource.com/emscripten-releases/+/8385c9ea32774b0bb8d205cc329c5db38d90da31 but I don't see that? Unless I'm missing something?

sbc100 commented 4 months ago

Yes, looks like this 3.1.56 release does not include https://github.com/emscripten-core/emscripten/pull/21523.. not sure how the CI passed.

aheejin commented 4 months ago

After https://github.com/emscripten-core/emscripten/pull/21523 landed in emscripten-release, it still failed saying 'data' was accessed before assigned or something. Turned out we needed https://github.com/emscripten-core/emscripten/pull/21525 too. So I waited until https://chromium.googlesource.com/emscripten-releases/+/db10d881cf7b6a94520c71fee14db0bcdd2c0bfb landed in emscripten-release and re-ran the CI here, and it passed.

(I also reran the CI of #1355, which seemed to pass)

aheejin commented 4 months ago

Oh wait, do you mean, passing the CI here is not sufficient, and the release itself has to contain the commits? In that case I have to redo the whole release with the different emscripten commit... Should I do that? Is that gonna be the problem?

sbc100 commented 4 months ago

Yes, you need create a new RC release and then scripts/create_release.py once that is ready.

I guess now we should call it 3.1.57 since 3.1.56 has now effectively been shipped.

sbc100 commented 4 months ago

I guess that tests in question here were somehow running against tot.. otherwise I don't see how they could have passed.

kripken commented 4 months ago

I guess now we should call it 3.1.57 since 3.1.56 has now effectively been shipped.

We can modify releases here, can't we? I think we did that in the past. It seems better if the short-term confusion is outweighed by what that commit fixes.

sbc100 commented 4 months ago

I guess now we should call it 3.1.57 since 3.1.56 has now effectively been shipped.

We can modify releases here, can't we? I think we did that in the past. It seems better if the short-term confusion is outweighed by what that commit fixes.

I think I would be OK with that. Some folks may already have downloaded the broken release.. but I think they should get updated if we update the hashes alone.

aheejin commented 4 months ago

I guess now we should call it 3.1.57 since 3.1.56 has now effectively been shipped.

We can modify releases here, can't we? I think we did that in the past. It seems better if the short-term confusion is outweighed by what that commit fixes.

You mean, manually only fix the emscripten hash here? But we should do that in emscripten-release too, no?

sbc100 commented 4 months ago

Yes, either way we need to trigger a new LTO build on emscripten-release

aheejin commented 4 months ago

OK will start a new one from scratch...

kripken commented 4 months ago

Yes, we need a new LTO build. But with that, we can trample this release. When people update their emsdk it will download the fixed release, so the confusion is only temporary.

aheejin commented 4 months ago

How can we "trample" this release?

sbc100 commented 4 months ago

How can we "trample" this release?

Basically you would re-run the scripts/create_release.py with the new hashes.

One option would be to revert the 7815dca change locally before you run the script. Another option would be to run the script and then edit the resulting change.

aheejin commented 4 months ago

Do you mean https://github.com/emscripten-core/emsdk/commit/da5a19215aa252067381d50c269055106e14b93e? https://github.com/emscripten-core/emsdk/commit/7815dcaa5c97ee387e925137a42dd4219e7cec82 is 3.1.55 release which you committed two weeks ago.

sbc100 commented 4 months ago

Sorry, yes.