search

murat-dogan / node-datachannel

WebRTC For Node.js and Electron. libdatachannel node bindings.
Mozilla Public License 2.0
308 stars 60 forks source link

Use C++ compiler for CMAKE_CXX_COMPILER env variable #282

Closed leblowl closed 2 months ago

leblowl commented 3 months ago

Fixes https://github.com/murat-dogan/node-datachannel/issues/279 for me on amd64 with gcc/g++

murat-dogan commented 2 months ago

Looks ok to me. Could you also please confirm? @funniray @SilentBot1

funniray commented 2 months ago

It looks fine to me, but I'm generally unsure what differences invoking g++/clang++ vs just gcc/clang are.

leblowl commented 2 months ago

@funniray for me, everything compiled fine, but I think it didn't link the c++ standard lib. The error was: undefined symbol: _ZTVN10__cxxabiv117__class_type_infoE

murat-dogan commented 2 months ago

@leblowl it seems you are right. When I build the lib locally, there is no error for electron. I have merged the PR, but have some errors on build. Could you please check it?

https://github.com/murat-dogan/node-datachannel/actions/runs/10864270307/job/30149535154

A note from CMake, from error

-- The C compiler identification is Clang 10.0.0
-- The CXX compiler identification is GNU 9.4.0

From a successful build

-- The C compiler identification is Clang 10.0.0
-- The CXX compiler identification is Clang 10.0.0
funniray commented 2 months ago

re-reviewed the commit and GXX isn't set in the environment variables here. https://github.com/leblowl/node-datachannel/blob/a4f744ceac3decdc3a3d5b7135dde60bb245a3c6/.github/workflows/build-linux.yml#L100

Should just have GXX: ${{ matrix.gxx }}. I completely overlooked it

leblowl commented 2 months ago

@funniray good catch! @murat-dogan I opened another PR with the fix: https://github.com/murat-dogan/node-datachannel/pull/291

murat-dogan commented 2 months ago

Unfortunately, this did not make any change—the same error. I tried without Clang++ but it also did not work. I reverted my changes.

murat-dogan commented 2 months ago

@funniray We have talked here about binary sizes, remember? https://github.com/murat-dogan/node-datachannel/pull/264#issuecomment-2185272483

Now with C++ compiler binary sizes are bigger again.

Compare here; https://github.com/murat-dogan/node-datachannel/releases/tag/v0.11.0 https://github.com/murat-dogan/node-datachannel/releases/tag/v0.12.0-dev

Probably c++ standard file was creating the diff.

funniray commented 2 months ago

Yeah, for x64 linux, the v0.12.0 build is much closer in size to the x64 linux v0.9.2 builds, so much of that size could've been from the C++ standard library somehow