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

Fixes compilation with Ninja and IntelLLVM when using OneAPI #544

Merged
merged 3 commits into from
May 26, 2024

Conversation

lmdiazangulo
Copy link
Contributor

@lmdiazangulo lmdiazangulo commented Oct 26, 2023

I have found some issues trying to compile with the recent Intel One API + Visual Studio 2022. Basically I got this error.

1> [CMake]   failed with:
1> [CMake] 
1> [CMake]    ninja: error: build.ninja:280: multiple rules generate jsonfortran.lib

I have implemented some changes in CMakeLists.txt to fix it by considering the case of IntelLLVM which is the compiler Id of the new compiler.

Best regards,
Luis

Copy link

codecov bot commented Dec 20, 2023

Codecov Report

Merging #544 (9eaa63f) into master (7819919) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #544   +/-   ##
=======================================
  Coverage   89.48%   89.48%           
=======================================
  Files           7        7           
  Lines        5356     5356           
=======================================
  Hits         4793     4793           
  Misses        563      563           

@jacobwilliams
Copy link
Owner

It looks like the CI failed? (haven't looked into why)

@jacobwilliams
Copy link
Owner

jacobwilliams commented May 25, 2024

What is the reason for both JSONFORTRAN_ENABLE_TESTS and ENABLE_TESTS? It's not clear to me what the user is supposed to set to get the tests...

(note: the CI is failing because the tests aren't being enabled)

@jacobwilliams
Copy link
Owner

It looks like -D ENABLE_TESTS=ON enables the tests?

@jacobwilliams jacobwilliams mentioned this pull request May 26, 2024
@jacobwilliams jacobwilliams merged commit f2eaeb0 into jacobwilliams:master May 26, 2024
5 of 6 checks passed
@lmdiazangulo
Copy link
Contributor Author

It looks like -D ENABLE_TESTS=ON enables the tests?

Yes. I added an option because at some point I was using it as a submodule which was imported with add_subdirectory from a parent cmake. It was a little bit confusing to me that the names in the variables defined by options do not include the name of the project they refer to.

Besides that, thank you a lot for this library. We are using it extensively in our FDTD solver project: https://github.com/OpenSEMBA/fdtd

@jacobwilliams
Copy link
Owner

I think these changes have broken the conda-forge package. The shared library doesn't seem to be being built anymore. Did that get disabled?

We should be getting:

-- Installing: $PREFIX/lib/libjsonfortran.so.8.4.0
-- Installing: $PREFIX/lib/libjsonfortran.so.8.4
-- Installing: $PREFIX/lib/libjsonfortran.so
-- Installing: $PREFIX/lib/libjsonfortran.a

Now we are only getting:

-- Installing: $PREFIX/lib/libjsonfortran.a

https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=943042&view=logs&j=656edd35-690f-5c53-9ba3-09c10d0bea97&t=986b1512-c876-5f92-0d81-ba851554a0a3

@jacobwilliams
Copy link
Owner

See #558

@lmdiazangulo
Copy link
Contributor Author

Yes, maybe I disabled that while doing some tests. Maybe is better to revert the merge. Sorry about that.

@jacobwilliams
Copy link
Owner

I think 1784d4e fixes it.

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.

None yet

2 participants