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

nodejs{12,14,16,18,20,22}: Use configure.cxxflags-append correctly #27285

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tuffnatty
Copy link
Contributor

Description

devel/nodejs* ports use configure.cxx-append to add C++ compiler flags. They also use compiler.blacklist-append to blacklist incompatible XCode versions. In such a situation, if compiler.blacklist-append is not placed before the first configure.cxx-append, on Mojave it results in configure.cxx not being set to the fallback compiler while configure.cc is correctly using the fallback compiler.

Failure logs (choose an OS version in the Port Health panel, look for CXX=, compare with CC=):

Solution: use configure.cxxflags-append instead.

While I was looking at this, I also re-enabled ccache usage, as the reports on V8 not liking ccache seem to be really outdated.

Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS 10.14.6 18G9323 x86_64
Command Line Tools 10.3.0.0.1.1562985497

Verification

Have you

  • followed our Commit Message Guidelines?
  • squashed and minimized your commits?
  • checked that there aren't other open pull requests for the same change?
  • referenced existing tickets on Trac with full URL in commit message?
  • checked your Portfile with port lint?
  • tried existing tests with sudo port test?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?
  • checked that the Portfile's most important variants haven't been broken?

@macportsbot
Copy link

Notifying maintainers:
@ci42 for port nodejs12, nodejs14, nodejs16, nodejs18, nodejs20, nodejs22.

@macportsbot macportsbot added type: bugfix maintainer: open Affects an openmaintainer port labels Jan 3, 2025
@kencu
Copy link
Contributor

kencu commented Jan 3, 2025

JFTR, I tried that first of course:

#26031 (comment)

but it would not build the test suite that way, and I had to change it to appending onto the CXX compiler instead.

It was building correctly for a while, and then things went kinda sideways with the nodejs port PR and I can't say what happened exactly after that. Might have gotten hosed up.

@kencu
Copy link
Contributor

kencu commented Jan 3, 2025

maybe we don't care about the test suite any more anyway. It crashed every single time I tried to run it, on every nodejs version.

Also, there are some reports that the nodejs ports that have been enabled for older systems are causing kernel panics:

https://trac.macports.org/ticket/71475

This is probably because the libuv embedded in these nodejs systems has not been modified to work properly on older systems like our libuv ports have been. I tried to see if I could get these nodejs ports to use our system libuv instead, but they would not.

The next step would be to go through each nodejs port and find the embedded libuv, and try to patch it according to what we do in the libuv port. But I have not got to that, nor has anyone else so far.

@tuffnatty
Copy link
Contributor Author

OK, I'm trying to run the tests at the moment. If we really need configure.cxx-append, let's move compiler.blacklist-append above it so that the fallback CXX is selected correctly.

I can't test the systems before 10.7 mentioned in https://trac.macports.org/ticket/71475 , so let's leave it for later.

@tuffnatty
Copy link
Contributor Author

tuffnatty commented Jan 4, 2025

Hi @kencu,
The tests do build successfully here on nodejs20 + Mojave. They do fail later when running, but they do so on a nodejs20 + Monterey with clean HEAD code as well, at the same place in the scripts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer: open Affects an openmaintainer port type: bugfix
Development

Successfully merging this pull request may close these issues.

4 participants