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

3.1.56 #1353

Merged
merged 5 commits into from
Mar 14, 2024
Merged

3.1.56 #1353

merged 5 commits into from
Mar 14, 2024

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented Mar 13, 2024

No description provided.

@aheejin aheejin requested a review from sbc100 March 13, 2024 17:14
@aheejin
Copy link
Member Author

aheejin commented Mar 13, 2024

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

@sbc100
Copy link
Collaborator

sbc100 commented Mar 13, 2024

Looks like an actual emscripten bug related to emscripten-core/emscripten#21456

@kripken
Copy link
Member

kripken commented Mar 13, 2024

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

@aheejin
Copy link
Member Author

aheejin commented Mar 14, 2024

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

@sbc100
Copy link
Collaborator

sbc100 commented Mar 14, 2024

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
Copy link
Member Author

aheejin commented Mar 14, 2024

Thank you!

@aheejin aheejin merged commit da5a192 into emscripten-core:main Mar 14, 2024
10 checks passed
@aheejin aheejin deleted the version_3.1.56 branch March 14, 2024 05:34
@sbc100
Copy link
Collaborator

sbc100 commented Mar 14, 2024

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
Copy link
Collaborator

sbc100 commented Mar 14, 2024

Yes, looks like this 3.1.56 release does not include emscripten-core/emscripten#21523.. not sure how the CI passed.

@aheejin
Copy link
Member Author

aheejin commented Mar 14, 2024

After emscripten-core/emscripten#21523 landed in emscripten-release, it still failed saying 'data' was accessed before assigned or something. Turned out we needed emscripten-core/emscripten#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
Copy link
Member Author

aheejin commented Mar 14, 2024

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
Copy link
Collaborator

sbc100 commented Mar 14, 2024

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
Copy link
Collaborator

sbc100 commented Mar 14, 2024

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

@kripken
Copy link
Member

kripken commented Mar 14, 2024

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 added a commit that referenced this pull request Mar 14, 2024
This means that CI run that update `latest` actually test the thing
we are about to ship.

We recently had a case where `latest` was broken but `tot` was fixes
and we accidentally shipped a broken SDK version (#1353).
@sbc100
Copy link
Collaborator

sbc100 commented Mar 14, 2024

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
Copy link
Member Author

aheejin commented Mar 14, 2024

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
Copy link
Collaborator

sbc100 commented Mar 14, 2024

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

@aheejin
Copy link
Member Author

aheejin commented Mar 14, 2024

OK will start a new one from scratch...

sbc100 added a commit that referenced this pull request Mar 14, 2024
This means that CI run that update `latest` actually test the thing
we are about to ship.

We recently had a case where `latest` was broken but `tot` was fixes
and we accidentally shipped a broken SDK version (#1353).
sbc100 added a commit that referenced this pull request Mar 14, 2024
This means that CI run that update `latest` actually test the thing
we are about to ship.

We recently had a case where `latest` was broken but `tot` was fixes
and we accidentally shipped a broken SDK version (#1353).
sbc100 added a commit that referenced this pull request Mar 14, 2024
This means that CI run that update `latest` actually test the thing
we are about to ship.

We recently had a case where `latest` was broken but `tot` was fixes
and we accidentally shipped a broken SDK version (#1353).
@kripken
Copy link
Member

kripken commented Mar 14, 2024

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
Copy link
Member Author

aheejin commented Mar 14, 2024

How can we "trample" this release?

@sbc100
Copy link
Collaborator

sbc100 commented Mar 14, 2024

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
Copy link
Member Author

aheejin commented Mar 14, 2024

Do you mean da5a192? 7815dca is 3.1.55 release which you committed two weeks ago.

@sbc100
Copy link
Collaborator

sbc100 commented Mar 14, 2024

Sorry, yes.

aheejin added a commit to aheejin/emsdk that referenced this pull request Mar 15, 2024
 emscripten-core#1353 turned out to be broken, so this is the second try.
aheejin added a commit that referenced this pull request Mar 15, 2024
#1353 turned out to be broken, so this is the second try.
aheejin added a commit to emscripten-core/emscripten that referenced this pull request Mar 15, 2024
After #21518 landed, the previous 3.1.56 release
(emscripten-core/emsdk#1353) turned out to be
broken so we replaced 3.1.56 with the new one
(emscripten-core/emsdk#1360). So here to be
precise we update the date of the new 3.1.56 release.
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.

3 participants