Skip to content

Refactor CMakeLists.txt #164

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

Merged

Conversation

anutosh491
Copy link
Collaborator

Removed redundancy and adds more consistency especially for the wasm build

@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.14%. Comparing base (a3b5cff) to head (83b3298).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #164   +/-   ##
=======================================
  Coverage   80.14%   80.14%           
=======================================
  Files          19       19           
  Lines         972      972           
  Branches       93       93           
=======================================
  Hits          779      779           
  Misses        193      193           

@anutosh491 anutosh491 marked this pull request as ready for review October 17, 2024 11:54
# ENV (https://github.com/emscripten-core/emscripten/commit/6d9681ad04f60b41ef6345ab06c29bbc9eeb84e0)
set(EMSCRIPTEN_FEATURES "${EMSCRIPTEN_FEATURES} -s \"EXTRA_EXPORTED_RUNTIME_METHODS=[ENV']\"")
endif()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are redundant as are being addressed through macro(xeus_cpp_set_common_options target_name) so can be totally removed from here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think some flags are still missing in the macro (-fexceptions and /EHsc, I may have missed others).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, having a look !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yupp those were the only ones. Should be ready now !

@anutosh491
Copy link
Collaborator Author

anutosh491 commented Oct 17, 2024

@vgvassilev this should be ready (P.S i made similar changes on xeus-r few days ago and I realized they would be applicable on xeus-cpp too so pretty confident :) )

@anutosh491 anutosh491 force-pushed the improve_cmakelists.txt branch from 974a851 to 81de961 Compare October 20, 2024 04:02
@@ -347,7 +332,6 @@ macro(xeus_cpp_create_target target_name linkage output_name)
# Curl initialised specifically for xassist
target_link_libraries(${target_name} PUBLIC ${XEUS_CPP_XEUS_TARGET} clangCppInterOp pugixml argparse::argparse curl)
else ()
# TODO : Add curl support for emscripten
Copy link
Collaborator Author

@anutosh491 anutosh491 Oct 20, 2024

Choose a reason for hiding this comment

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

Removed TODO which can't be resolved as mentioned here (emscripten-forge/recipes#1332 (comment))

The PR tried adding libcurl-static on emscripten-forge but the above comment explains why the recipe can't be put to use.

@JohanMabille JohanMabille merged commit a173037 into compiler-research:main Oct 21, 2024
8 checks passed
@anutosh491 anutosh491 deleted the improve_cmakelists.txt branch October 21, 2024 13:30
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