-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
emsdk: add new version 3.1.72 #24218
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Minor change, otherwise looks good!
These are the failing configurations: https://c3i.jfrog.io/c3i/misc/summary.html?json=https://c3i.jfrog.io/c3i/misc/logs/pr/24218/1-macos-m1-clang/emsdk/2.0.34//summary.json |
@RubenRBS |
Let me know if you need any help from my side! |
This comment has been minimized.
This comment has been minimized.
Umm, strange. @RubenRBS emsdk/3.1.6 build on macOSX apple-clang 15.0.0======== Exporting recipe to the cache ======== ======== Input profiles ======== Profile build: ======== Computing dependency graph ======== ======== Computing necessary packages ======== ======== Installing packages ======== -------- Installing package emsdk/3.1.61 (2 of 2) -------- emsdk/3.1.61: RUN: ./emsdk activate releases-upstream-28e4a74b579b4157bda5fc34f23c7d3905a8bd6c-64bit Next steps:
emsdk/3.1.61: Package 'dcf68e932572755309a5f69f3cee1bede410e907' built emsdk/3.1.61: package(): Packaged 68 '.txt' files ======== Launching test_package ======== ======== Computing dependency graph ======== ======== Computing necessary packages ======== ======== Installing packages ======== ======== Testing the package ======== ======== Testing the package: Building ======== ======== Testing the package: Executing test ======== emsdk/3.1.61 (test package): RUN: em++ -v |
LGTM, you may have to also bump Node to 18.20.3 though. That's the LTS that currently ships via emsdk latest. 18.15.0 would ship with Conan and is probably good enough. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@@ -29,17 +31,29 @@ def layout(self): | |||
basic_layout(self, src_folder="src") | |||
|
|||
def requirements(self): | |||
self.requires("nodejs/16.3.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There has been some talk about this recipe requiring nodejs directly, instead of it being a tool_requires, we should clarify if it's really needed or if we could get away with tool_requireing it.
The issue is that users of emsdk are not able to override nodejs, and they get conflicts using it as a tool_requires
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
recipes/emsdk/all/conandata.yml
Outdated
@@ -1,4 +1,7 @@ | |||
sources: | |||
"3.1.69": | |||
url: "https://github.com/emscripten-core/emsdk/archive/3.1.60.tar.gz" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something I noticed while I was checking this version update is that the source version is updated, but this url is still pointing to version 3.1.60.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SonarShroom
Thank you for your comment!
I fixed url and sha256.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 4832d21emsdk/3.1.49@#348629534e32711e1a777cc07e9882bc
emsdk/3.1.31@#4b39caf3eb422cc57e1179b1b1a7f3e1
emsdk/3.1.47@#92e1654b9bd859a740a7d96dec30aead
emsdk/3.1.50@#cccc04fd1df5316a6c4e852900ed3e2c
emsdk/3.1.45@#1e0a4f2704b7ceb7248102335cd7d876
emsdk/3.1.70@#be940c724e6fc22e65cc0b8876317840
emsdk/3.1.48@#7ea1b4055b543b49928c452932978bfe
emsdk/3.1.46@#3dcc05e86457773abd7566b18b6feeee
emsdk/3.1.18@#86da377ddee4da01b1a270973f04a542
emsdk/3.1.16@#72b2410796726209a8a1e208d4bfa6d2
emsdk/3.1.17@#3c12f716d19e58051174542297c1f323
emsdk/3.1.7@#53d6302dee97bc934a126e48ea41ea3b
emsdk/3.1.29@#8f67d65544f1fd8ef987393d57c82fad
emsdk/3.1.12@#fc9e2625bd08c498634616f9180798e0
emsdk/3.1.23@#80fff9d3a25ad2399a0539b51b8db284
emsdk/3.1.20@#c773ee87bb1313d7d5e4cdbecb777e59
emsdk/3.0.1@#f690e09e5c086fabb2967053d2eb2c9c
emsdk/2.0.34@#47cfe0b3c1188e359f5c063c7efefd22
emsdk/2.0.30@#b1e90e680fe3b9a579195c9600033250
emsdk/3.1.30@#6fa4ad840f965a88646c6268249a58b0
emsdk/3.1.5@#fcb3b244bbde486c3caac0df6dcf7fb7
emsdk/3.1.4@#3960d418a596f187aed363a0def34315
emsdk/3.1.3@#ae5e5a7d767f121ee13e890e3c1a740e
emsdk/3.1.44@#acdf7b8a651882d781277f1a39657df1
emsdk/3.0.0@#b565357a023010ba6e7fe5a768bb4afc
emsdk/3.1.15@#46ca73af886acaf449631e01e7926c5f
emsdk/2.0.31@#1ee4e2b2cdfcdbd28b3f416db87c5aaf
emsdk/3.1.0@#f80135ff8195559d4aa75eb182cea1fc
|
Conan v1 pipeline ✔️Warning Conan Center will stop receiving updates for Conan 1.x packages soon - please see announcement. All green in build 12 (
Conan v2 pipeline ✔️
All green in build 12 (
|
Obviously I'm not a maintainer for this package manager, but I would like to ask if the review period for this specific package takes long? There's at least #22680 which has not seen as much activity asking for pretty much the same package update. Seeing as @toge has implemented the changes indicated, would there be any other kind of work (like testing) required? |
@SonarShroom |
The issue here is the usage of Not quite sure what the best approach would be in this case, if only dropping the check would help, I'll need to ask @jcar87 |
Those sort of platform dependen checks in
That's a general Conan expectation and not a specific of our CI pipeline - adding platform specific logic outside of settings and options has always been discouraged AFAIK (in the
My advice right now would be to remove that logic from the recipe and let CI fail so that we can have a look at the issue. |
Specify library name and version: emsdk/3.1.72
emscripten-core/emsdk@3.1.50...3.1.72