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

[port] [contrib.glfw3] Add compile time define #21797

Merged
merged 2 commits into from
Apr 24, 2024

Conversation

ypujante
Copy link
Contributor

@ypujante ypujante commented Apr 22, 2024

In a project that wants to use my port, the issue is that the user can use either the built-in glfw3 port (-sUSE_GLFW=3) or the new one (--use-port=contrib.glfw3). There is a (somewhat hacky) way to figure out at runtime which port is being used (using the glfwGetVersionString() call). But there is no compile time way. And since this project does not want to force their user to use the new port, there needs to be a way to distinguish at compile time...

Hence this PR which adds the define EMSCRIPTEN_USE_PORT_CONTRIB_GLFW3 when compiling with the new port.

// example
./emcc --verbose --use-port=contrib.glfw3 ./test/hello_world.cpp -o /tmp/index.js
 "/usr/local/emsdk/upstream/bin/clang-19" -cc1 -triple wasm32-unknown-emscripten -emit-obj -mrelax-all 
-disable-free -clear-ast-before-backend -disable-llvm-verifier -discard-value-names -main-file-name 
hello_world.cpp -mrelocation-model static -mframe-pointer=none -ffp-contract=on -fno-rounding-math 
-mconstructor-aliases -target-cpu generic -debugger-tuning=gdb -fdebug-compilation-dir=/usr/local/emsdk/emscripten/main 
-v -fcoverage-compilation-dir=/usr/local/emsdk/emscripten/main -resource-dir /usr/local/emsdk/upstream/lib/clang/19 -isystem /usr/local/emsdk/emscripten/main/cache/sysroot/include/contrib.glfw3 
-D EMSCRIPTEN -D EMSCRIPTEN_USE_PORT_CONTRIB_GLFW3 
-isysroot /usr/local/emsdk/emscripten/main/cache/sysroot -I/usr/local/include -internal-isystem /usr/local/emsdk/emscripten/main/cache/sysroot/include/wasm32-emscripten/c++/v1 
-internal-isystem /usr/local/emsdk/emscripten/main/cache/sysroot/include/c++/v1 
-internal-isystem /usr/local/emsdk/upstream/lib/clang/19/include 
-internal-isystem /usr/local/emsdk/emscripten/main/cache/sysroot/include/wasm32-emscripten 
-internal-isystem /usr/local/emsdk/emscripten/main/cache/sysroot/include -Werror=implicit-function-declaration 
-fdeprecated-macro -ferror-limit 19 -fvisibility=default -fgnuc-version=4.2.1 -fskip-odr-check-in-gmf 
-fcxx-exceptions -fignore-exceptions -fexceptions -fcolor-diagnostics -iwithsysroot/include/fakesdl 
-iwithsysroot/include/compat -mllvm -combiner-global-alias-analysis=false -mllvm -enable-emscripten-sjlj 
-mllvm -disable-lsr -o /var/folders/wz/zqhny4ks031bqlr6r27bfkzm0000gn/T/emscripten_temp_obwaqlpy/hello_world_0.o -x c++ ./test/hello_world.cpp

@ypujante
Copy link
Contributor Author

Failing tests are:

# test-other
======================================================================
FAIL [0.001s]: test_pthread_print_override_modularize (test_other.other)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python3.6/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/usr/lib/python3.6/unittest/case.py", line 605, in run
    testMethod()
  File "/root/project/test/common.py", line 324, in decorated
    f(self, *args, **kwargs)
  File "/root/project/test/common.py", line 175, in modified
    raise AssertionError('Flaky test has failed too many times') from preserved_exc
AssertionError: Flaky test has failed too many times

# test-browser-chrome
======================================================================
FAIL [3.043s]: test_wasmfs_opfs_jspi (test_browser.browser)
----------------------------------------------------------------------
...
======================================================================
FAIL [3.028s]: test_wasmfs_opfs_jspi_wasm_bigint (test_browser.browser)
----------------------------------------------------------------------
...

They are unrelated to my changes

@ypujante
Copy link
Contributor Author

I merged with main branch and now all tests are passing.

@Jeremy-Boyle
Copy link

Hey @Jeremy-Boyle , can you delete this comment and post it on the pongasoft/emscripten-glfw project instead? I don't want to pollute this PR with unrelated items. Thank you

I was able to figure it out, comment removed due to it being unrelated to this PR.

@sbc100
Copy link
Collaborator

sbc100 commented Apr 24, 2024

Could you perhaps just define this, or something like it, in your glfw header?

@ypujante
Copy link
Contributor Author

Hi @sbc100

I am not sure which header you are talking about:

  • if it is the main header (GLFW/glfw3.h), then although it is possible, I would prefer not to modify it. It would also not cover the use case of a user using their own local copy of glfw headers (and using -I my_local_copy_of_glfw)
  • if it is GLFW/emscripten_glfw3.h which is specific to my project, then it is a chicken and egg problem because you don't know if this header will be present. It won't be present if using -sUSE_GLFW=3 and so you cannot include it...

Maybe there is a 3rd alternative. I suppose I could add and modify GLFW/glfw3native.h which is a standard glfw header and add a section like this:

// in GLFW/glfw3native.h
#if defined(GLFW_EXPOSE_NATIVE_EMSCRIPTEN)
#define EMSCRIPTEN_USE_PORT_CONTRIB_GLFW3
// also add the definitions that are currently under GLFW/emscripten_glfw3.h here and get rid of this include entirely...
#endif

then the calling code would use it like this:

#define GLFW_EXPOSE_NATIVE_EMSCRIPTEN
#include <GLFW/glfw3native.h>

#ifdef EMSCRIPTEN_USE_PORT_CONTRIB_GLFW3
... whatever code is specific to the contrib port
#endif

I suppose there is still the potential issue that a user could have their own local copy of glfw . But I am open for such an option if you think that is a better avenue.

@sbc100
Copy link
Collaborator

sbc100 commented Apr 24, 2024

I don't have a strong opinion. I seems easy to modify your glfw3.h and I don't think you need to be supporting folks that want to use their own header with your version of the library.. that sounds strange. However, we also just land this if you prefer.

@ypujante
Copy link
Contributor Author

Since this change is isolated to contrib/glfw3.py and simply add a define to the command line, I can still work on the other solution of changing "GLFW/glfw3.h" and adding the define there, and then remove the define on the command line when I push the port change for the new version of the contrib port: that would be a totally transparent change to the user.

So yes, if you could merge this now so that it unblocks my progress with the other PR in ImGui. And then I can revisit it in another PR later on.

Thank you for your feedback and point of view. It is always much appreciated.

@sbc100 sbc100 merged commit 605d974 into emscripten-core:main Apr 24, 2024
29 checks passed
@ypujante ypujante deleted the glfw3-contrib-port-define branch December 15, 2024 18:18
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.

3 participants