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

Integrate npm package publish #2184

Merged
merged 5 commits into from
Dec 12, 2024
Merged

Conversation

jellespijker
Copy link
Member

@jellespijker jellespijker commented Dec 10, 2024

Publish a NPM package from a conan recipe.

This is based on main from the work done on #2183

Renamed the conan-package.yml workflow to package.yml since it is now also used for NPM packages

Added a conf to the recipe, which is used by the conan npm generator to generate a package.json. NPM packages are created on pushes to main, NP-* and 5.10 release branch and they will end up here: https://github.com/Ultimaker/CuraEngine/pkgs/npm/curaenginejs

Yes, the hardcoded 5.10 hurts.
Yes, the replacement of the + to - in the version of the package.json hurts, but NPM kept "sanitizing" the build-meta data away and the version needs to be unique.

See also:

Checklist:

  • My code follows the style guidelines of this project as described in UltiMaker Meta
  • I have read the Contribution guide
  • I have commented my code, particularly in hard-to-understand areas
  • I have uploaded any files required to test this change

Use the new workflows for Conan v2 to build and publish
and npm package

Contribute to NP-637
@jellespijker jellespijker mentioned this pull request Dec 10, 2024
4 tasks
Copy link
Contributor

github-actions bot commented Dec 10, 2024

Test Results

27 tests   27 ✅  5s ⏱️
 1 suites   0 💤
 1 files     0 ❌

Results for commit 2095fcc.

♻️ This comment has been updated with latest results.

Copy link
Member

@rburema rburema left a comment

Choose a reason for hiding this comment

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

lgtm, but I do have a few suggestions

conanfile.py Outdated Show resolved Hide resolved
.github/workflows/package.yml Outdated Show resolved Hide resolved
.github/workflows/package.yml Outdated Show resolved Hide resolved
The npmpackage Python requirement was introduced to handle package.json configuration for Emscripten builds. This simplifies the code by delegating the package.json setup to the npmpackage module, reducing redundancy and maintaining cleaner logic.

Contribute to NP-637
Added checks to validate minimum compiler versions and enforce C++20 compatibility for the package. These include GCC 12, Clang 14, Apple Clang 13, MSVC 191, and Visual Studio 17. This ensures compatibility with the required C++ features, preventing builds with unsupported compilers.

Contribute to NP-637
Contribute to NP-637
Contribute to NP-637
Copy link
Contributor

@wawanbreton wawanbreton left a comment

Choose a reason for hiding this comment

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

One remark, looks good otherwise

@@ -22,7 +23,7 @@ class CuraEngineConan(ConanFile):
exports = "LICENSE*"
settings = "os", "compiler", "build_type", "arch"
package_type = "application"
python_requires = "sentrylibrary/1.0.0@ultimaker/stable"
python_requires = "sentrylibrary/1.0.0@ultimaker/stable", "npmpackage/[>=1.0.0]@ultimaker/np_637"
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess there is no way to have this dependend on settings.os ?

@jellespijker jellespijker merged commit 2095fcc into main Dec 12, 2024
16 checks passed
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.

4 participants