Prepare nodejs for v18 #16966

KozhinovAlexander · 2023-04-11T07:24:37Z

Prepare nodjs package for version 18

I've read the contributing guidelines.
I've used a recent Conan client version close to the currently deployed.
I've tried at least one configuration locally with the conan-center hook activated.

ghost · 2023-04-11T07:35:44Z

I detected other pull requests that are modifying nodejs/all recipe:

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

prince-chrismc

Thanks for taking the time to submit this PR, there's a quiet a fe things i was not expecting so there are lots of questions if you dont mind answering that would be great

recipes/nodejs/all/conanfile.py

prince-chrismc · 2023-04-13T21:11:24Z

recipes/nodejs/all/conanfile.py


    @property
    def _source_subfolder(self):
        return os.path.join(self.source_folder, "source_subfolder")

    @property
    def _nodejs_arch(self):
-        if str(self.settings.os) == "Linux":
-            if str(self.settings.arch).startswith("armv7"):
+        if str(self.info.settings.os) == "Linux":


self.info should only be used in package_info this looks very bizarre 🤔

See the reason here: conan-io/conan#13506 Basically it is a bug in conan itself ;)

No no, it's usually a sign of doing something funky that probably has a different solution.

There are checks that need to happen in both generate and package_id

prince-chrismc · 2023-04-13T21:11:35Z

recipes/nodejs/all/conanfile.py

+        return str(self.info.settings.arch)
+
+    @property
+    def _glibc_version(self):


Why is this required?

Test will fail otherwise. See supported platforms here: https://github.com/nodejs/node/blob/main/BUILDING.md#platform-list It states glibc>=2.17

prince-chrismc · 2023-04-13T21:12:12Z

recipes/nodejs/all/conanfile.py

-            not (str(self.settings.os) in self.conan_data["sources"][self.version]) or
-            not (self._nodejs_arch in self.conan_data["sources"][self.version][str(self.settings.os)] ) ):
+        if not self.version in self.conan_data["sources"] or \
+           not str(self.info.settings.os) in self.conan_data["sources"][self.version] or \


info should not be add here 🤔

See the reason here: conan-io/conan#13506 Basically it is a bug in conan itself ;)

No no, that issue was specific to using it in package for validate the recommendation is to access self.settings directly.

recipes/nodejs/all/conanfile.py

recipes/nodejs/all/test_package/conanfile.py

KozhinovAlexander · 2023-04-13T21:45:37Z

Thanks for taking the time to submit this PR, there's a quiet a fe things i was not expecting so there are lots of questions if you dont mind answering that would be great

Thank you for your review. I've added some changes and comments. The usage self.info.settings is commented with reference to the thread with a bug in conan itself. Please consider it as a necessary workaround at the moment. Otherwise I do not know, how to push this PR through build stage. And waiting half a year for a bugfix is somehow not works for me :)

Signed-off-by: Alexander Kozhinov <[email protected]>

prince-chrismc · 2023-04-28T20:19:09Z

Oh wow. the recipe was just bad before? pre compiled binaries should be downloaded in the build folder not the source method

Check the cmake recipe for an example

conan-center-index/recipes/cmake/binary/conanfile.py

Line 30 in 2dc2a4f

    
           get(self, **self.conan_data["sources"][self.version][str(self.settings.os)][arch],

recipes/nodejs/all/conanfile.py

Co-authored-by: Chris Mc <[email protected]>

Signed-off-by: Alexander Kozhinov <[email protected]>

KozhinovAlexander · 2023-04-28T20:53:15Z

Oh wow. the recipe was just bad before? pre compiled binaries should be downloaded in the build folder not the source method

Check the cmake recipe for an example

conan-center-index/recipes/cmake/binary/conanfile.py

Line 30 in 2dc2a4f

get(self, **self.conan_data["sources"][self.version][str(self.settings.os)][arch],

Sometimes I am wondering the restrictions you tell me :) I've never seen thouse in conan docs. And I am using conan v1 already 1,5 years in my company :D

Signed-off-by: Alexander Kozhinov <[email protected]>

conan-center-bot · 2023-04-28T22:57:37Z

Conan v1 pipeline ✔️

All green in build 43 (7650f237cb9548d7c61ccdac6ac685eef9c47648):

nodejs/12.14.1@:
All packages built successfully! (All logs)
nodejs/13.6.0@:
All packages built successfully! (All logs)
nodejs/16.3.0@:
All packages built successfully! (All logs)

Conan v2 pipeline ✔️

Note: Conan v2 builds may be required once they are on the v2 ready list

All green in build 43 (7650f237cb9548d7c61ccdac6ac685eef9c47648):

nodejs/12.14.1@:
All packages built successfully! (All logs)
nodejs/16.3.0@:
All packages built successfully! (All logs)
nodejs/13.6.0@:
All packages built successfully! (All logs)

gjasny · 2023-04-29T10:20:00Z

Sometimes I am wondering the restrictions you tell me :) I've never seen thouse in conan docs. And I am using conan v1 already 1,5 years in my company :D

Actually, there is a package template named prebuild_tool_package, which does it the same way:

conan-center-index/docs/package_templates/prebuilt_tool_package/all/conanfile.py

Lines 36 to 46 in eccffc6

    
           # do not cache as source, instead, use build folder 
        
           def source(self): 
        
               pass 
        
           # download the source here, than copy to package folder 
        
           def build(self): 
        
               get( 
        
                   self, 
        
                   **self.conan_data["sources"][self.version][str(self.settings.os)][str(self.settings.arch)], 
        
                   strip_root=True, 
        
               )

I always try to follow one of the templates.

gjasny · 2023-04-29T10:23:31Z

recipes/nodejs/all/test_package/conanfile.py

            self.output.info("Node version:")
-            self.run("node --version", run_environment=True)
+            self.run("node --version")


Suggested change

self.run("node --version")

self.run("node --version")

self.run("npm --version")

self.run("npx --version")

gjasny · 2023-04-29T10:23:50Z

recipes/nodejs/all/test_v1_package/conanfile.py

+    def test(self):
+        if not cross_building(self):
+            self.output.info("Node version:")
+            self.run("node --version")


Suggested change

self.run("node --version")

self.run("node --version")

self.run("npm --version")

self.run("npx --version")

gjasny · 2023-04-29T10:26:47Z

And thank you for all your work and patience! 🐝

prince-chrismc · 2023-05-01T21:16:44Z

Sometimes I am wondering the restrictions you tell me :) I've never seen thouse in conan docs. And I am using conan v1 already 1,5 years in my company :D

The biggest sin of 1.x was letting you do anything even thought it was not recommended or error prone.
In CCI we have 100s of PRs a week and to double checking every corner case is just not possible...
I personally just ask author to change it to the way I know it's going to work so we do not have awkward work arounds... just because you can does not mean you should.

Reviewing PRs is not my job but I actually enjoy it so I make a few hours a week to do them.

As for the docs, they are never complete of finished so there's a done of things missing. Completely features are just undocumented still so as much as they are a source of truth they are sometimes missing things.

On the flip side CCI's community has a difference of opinion when it comes to writing recipes, not all of the conventions here are the "recommendation" from Conan itself... for example how we apply patches... it's easier to review and maintain the CCI way but not necessarily required.

But it's super hard to explain and looks and review for corner cases.

Lastly you have all the enterprise users who build packages from CCI, they prefer constancy so they can more easily patch and fix large amount of recipes at once. We do not support this but the more uniform a recipe gets the easier they re to wrk with.

KozhinovAlexander · 2023-05-01T21:37:04Z

Sometimes I am wondering the restrictions you tell me :) I've never seen thouse in conan docs. And I am using conan v1 already 1,5 years in my company :D

The biggest sin of 1.x was letting you do anything even thought it was not recommended or error prone.
In CCI we have 100s of PRs a week and to double checking every corner case is just not possible...
I personally just ask author to change it to the way I know it's going to work so we do not have awkward work arounds... just because you can does not mean you should.

Reviewing PRs is not my job but I actually enjoy it so I make a few hours a week to do them.

As for the docs, they are never complete of finished so there's a done of things missing. Completely features are just undocumented still so as much as they are a source of truth they are sometimes missing things.

On the flip side CCI's community has a difference of opinion when it comes to writing recipes, not all of the conventions here are the "recommendation" from Conan itself... for example how we apply patches... it's easier to review and maintain the CCI way but not necessarily required.

But it's super hard to explain and looks and review for corner cases.

Lastly you have all the enterprise users who build packages from CCI, they prefer constancy so they can more easily patch and fix large amount of recipes at once. We do not support this but the more uniform a recipe gets the easier they re to wrk with.

Thank you very much for detailed explanation and taking time for review. Basically I am okay with it and happy to do the things right way. Some stuff was just very new to me, I've never seen in that context. But now, I can say, that a lot of developers writing recipes ...hmmh... not completely right :D

prince-chrismc · 2023-05-01T22:22:12Z

❤️ Just trying to fill in the context since this project has changed over the last almost 4 years :)

KozhinovAlexander · 2023-05-02T11:41:22Z

@gjasny @SSE4 would you please approve?

* nodejs: conanfile.py: prepare for v18 Signed-off-by: Alexander Kozhinov <[email protected]> * test_package: conanfile.py: update for conan v2 usage Signed-off-by: Alexander Kozhinov <[email protected]> * Update recipes/nodejs/all/conanfile.py Co-authored-by: Chris Mc <[email protected]> * nodejs: conanfile.py: remove compiler verification Signed-off-by: Alexander Kozhinov <[email protected]> * nodejs: test_package: rework for v1 and v2 Signed-off-by: Alexander Kozhinov <[email protected]> * Update recipes/nodejs/all/conanfile.py Co-authored-by: Chris Mc <[email protected]> * nodejse: conanfile.py: rework conan_version test Signed-off-by: Alexander Kozhinov <[email protected]> * Update recipes/nodejs/all/conanfile.py Co-authored-by: Gregor Jasny <[email protected]> * Update recipes/nodejs/all/test_package/conanfile.py Co-authored-by: Gregor Jasny <[email protected]> * Update recipes/nodejs/all/conanfile.py Co-authored-by: Gregor Jasny <[email protected]> * Update recipes/nodejs/all/conanfile.py Co-authored-by: Gregor Jasny <[email protected]> * nodejs: conanfile.py: use self.settings instead of self.info.settings Signed-off-by: Alexander Kozhinov <[email protected]> * nodejs: conanfile.py: do not use self.settings in source() method Signed-off-by: Alexander Kozhinov <[email protected]> * nodejs: conanfile.py: use subprocess in _glibc_version Signed-off-by: Alexander Kozhinov <[email protected]> * Update recipes/nodejs/all/conanfile.py Co-authored-by: Chris Mc <[email protected]> * Update recipes/nodejs/all/conanfile.py Co-authored-by: Chris Mc <[email protected]> * Update recipes/nodejs/all/conanfile.py Co-authored-by: Chris Mc <[email protected]> * Update recipes/nodejs/all/conanfile.py Co-authored-by: Chris Mc <[email protected]> * nodejs: conanfile.py: fix arch / revert _glibc_version Signed-off-by: Alexander Kozhinov <[email protected]> * nodejs: conanfile.py: remove dobule defined build() method Signed-off-by: Alexander Kozhinov <[email protected]> * nodejs: conanfile.py: fix glibc version aquisition Signed-off-by: Alexander Kozhinov <[email protected]> --------- Signed-off-by: Alexander Kozhinov <[email protected]> Co-authored-by: Chris Mc <[email protected]> Co-authored-by: Gregor Jasny <[email protected]>

ghost mentioned this pull request Apr 11, 2023

[service] Potentially conflicting recipes #3571

Closed

This was referenced Apr 11, 2023

Package/nodejs v18 #16964

Merged

feat(nodejs): make recipe Conan 2.0 ready #16908

Closed

prince-chrismc mentioned this pull request Apr 11, 2023

Pull Requests Ready to be Reviewed prince-chrismc/conan-center-index-pending-review#1

Open

conan-center-bot added the Failed label Apr 11, 2023