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

CMake: misc #119

Closed
wants to merge 2 commits into from
Closed

CMake: misc #119

wants to merge 2 commits into from

Conversation

jschueller
Copy link
Collaborator

@jschueller jschueller commented Dec 1, 2023

  • Set C_STANDARD locally
  • Split primac fortran sources in a separate library (not supported in some msvc configurations)
  • Use generator expression for example location

@nbelakovski
Copy link
Contributor

I'm not opposed to this, since I believe modern CMake prefers the user specify things like this on targets as opposed to globally, but I'm wondering why you're proposing this at this time? Is there some bug you're seeing that this solves or is it just a cleanup PR?

@jschueller
Copy link
Collaborator Author

just tyding things up

@nbelakovski
Copy link
Contributor

Looks like it's causing some build failures on intel classic on macos because the standard was not specified for the stress_c_exe target, can you specify C99 for that target and also for data_c_exe? Also if you rebase off the latest main the windows gcc11 build should work.

CMakeLists.txt Fixed Show fixed Hide fixed
c/CMakeLists.txt Fixed Show fixed Hide fixed
c/CMakeLists.txt Fixed Show fixed Hide fixed
c/CMakeLists.txt Fixed Show fixed Hide fixed
c/CMakeLists.txt Fixed Show fixed Hide fixed
@jschueller jschueller changed the title CMake: Set C_STANDARD locally CMake: misc Dec 26, 2023
@zaikunzhang
Copy link
Member

Thank you @jschueller for proposing this. Ping me when you and @nbelakovski think it is ready.

@jschueller
Copy link
Collaborator Author

its ready from my side, @nbelakovski I added a commit to split the fortran/c sources into separate libs

@nbelakovski
Copy link
Contributor

@jschueller Can you make a comment (preferrably in the git commit but in the PR is fine too) as to why the C/F sources were split up? I know there was some discussion about it in a separate PR but I just want to make sure we have traceability as to the motivation for any changes.

@jschueller jschueller force-pushed the patch-1 branch 2 times, most recently from 8b2af98 to b839d46 Compare December 28, 2023 10:02
@jschueller
Copy link
Collaborator Author

done

@nbelakovski
Copy link
Contributor

nbelakovski commented Dec 28, 2023

@zaikunzhang I believe this PR is good to go.

@zaikunzhang
Copy link
Member

Thank you @jschueller and @nbelakovski.

I have got two questions.

Split primac fortran sources in a separate library (not supported in some msvc configurations)

Does this mean that the previous setup was not supported in some msvc configurations, but the current one is supported?

Do we need to revise the CMake snippets in README?

Thank you.

@nbelakovski
Copy link
Contributor

Does this mean that the previous setup was not supported in some msvc configurations, but the current one is supported?

My understanding is yes.

Do we need to revise the CMake snippets in README?

I've reviewed the README and I don't think we need to update it, but maybe @jschueller would like to review it and come to his own conclusion.

I do think we should add some tests in order to validate this. Right now all tests override the default CMake generator by providing -G Ninja. I think we should add a test that looks similar to the one in which we override the C compiler for ifx. It could look something like this (these are additions to cmake.yml)

cmake-main:
    runs-on: ${{ matrix.os }}
    env:
      GENERATOR: "Ninja"

...

- os: windows-latest
            toolchain: {compiler: intel, version: '2023.2', cflags: '', fflags: '/warn:all /debug:extended /Z7 /fimplicit-none /standard-semantics /assume:recursion' generator: "Visual Studio 17 2022" }

...

- name: Override CMake generator
        if: ${{ matrix.toolchain.generator }}
        run: echo "GENERATOR=${{ matrix.toolchain.generator}}" >> $env:GITHUB_ENV

...
    cmake --version
    cmake -G $GENERATOR -DCMAKE_BUILD_TYPE=RelWithDebInfo ...

I haven't tested this, but this is the general idea.

@jschueller jschueller force-pushed the patch-1 branch 10 times, most recently from 64839f7 to 969d471 Compare December 29, 2023 15:57
@jschueller jschueller force-pushed the patch-1 branch 2 times, most recently from 740561a to 78ef3fc Compare December 29, 2023 16:15
.github/workflows/cmake.yml Fixed Show fixed Hide fixed
.github/workflows/cmake.yml Fixed Show fixed Hide fixed
@jschueller
Copy link
Collaborator Author

I cannot make it work with the vs generator, but succeeded with the nmake generator

@nbelakovski
Copy link
Contributor

I also tried to get it to work with the vs generator on my machine and couldn't do it. It seems others have issues with this as well fortran-lang/setup-fortran#45 (and the proposed solution in that thread, to use the install-intelfortran-action, doesn't work as it is being deprecated in favor of setup-fortran).

I'm not familiar with NMake. Is it part of Visual Studio? This also raises the question for me as to whether or not NMake requires the sources to be separated? I think it would be preferable to have fewer libraries so as to have fewer artifacts to move around for the various packaging systems we plan to support.

@jschueller
Copy link
Collaborator Author

jschueller commented Dec 30, 2023

there are no additional files as its an object-only library, so it doesnt hurt

nmake is part of ms tools yes, but it doesnt require sources to be separated

@zaikunzhang
Copy link
Member

Hi @jschueller and @nbelakovski, has your discussion reached a conclusion? What about the tests mentioned by @nbelakovski ? Thank you.

@jschueller
Copy link
Collaborator Author

jschueller commented Jan 2, 2024

it seems we cannot actualy test it with the visual studio generator atm as it yields another unknown error (possibly unrelated)

@nbelakovski
Copy link
Contributor

@zaikunzhang @jschueller Since NMake doesn't require sources to be separated I think we should undo the changes in this PR that separate the sources. My rationale is that the code was modified to achieve a specific goal (using Visual Studio CMake generator) and then the goal was changed (to supporting NMake), so the only changes that should be in this PR are those required to support NMake (as well as the change to move C standard from a global definition to a target-specific one). I think (I could be wrong here) that it already compiles with NMake, so the only changes would be the change to C standard and also to cmake.yml to ensure that NMake works and continues to work in the future.

@jschueller
Copy link
Collaborator Author

jschueller commented Jan 4, 2024

yes I removed it because the goal was not nmake, and in fact we dont care about nmake
but the commit to split c/fortran sources is still a valid idea for visual, as reported by @TLCFEM

@nbelakovski
Copy link
Contributor

yes I removed it because the goal was not nmake, and in fact we dont care about nmake

I'm confused, what did you remove?

but the commit to split c/fortran sources is still a valid idea for visual, as reported by @TLCFEM

It may be a valid idea, but neither you or I were actually able to get it to work. Call me conservative, but I think that if we try an idea, and we can't get it to work, we should roll back the changes that were made in that direction. Otherwise we get these sections in the codebase whose history is "this was changed to try something which didn't work" and when someone is modifying or even just reading the code at a later date it's very confusing.

I'm not opposed to splitting up C/Fortran sources in principle, I just want to keep this PR focused and well-scoped.

@jschueller
Copy link
Collaborator Author

@jschueller jschueller force-pushed the patch-1 branch 2 times, most recently from 52f8559 to 0a7a62e Compare January 7, 2024 09:50
target_include_directories (primac PUBLIC
$<INSTALL_INTERFACE:include>
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>)
target_link_libraries (primac PUBLIC primaf) # must be PUBLIC for precision macros
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is CONFIG added?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's to find .mod files in vs, which adds one dir per config

@nbelakovski
Copy link
Contributor

It's not that I don't believe you that the VS generator doesn't support mixed sources, I just don't like the idea of doing part of the work to support the VS generator and not doing the rest of it, particularly when that work isn't just cleanup but more along the lines of re-arranging things.

That being said, if you insist on this then I guess it doesn't hurt, but in the unlikely event that this causes issues with some future changes I think it should be considered acceptable to re-merge the sources (assuming we're not actively supporting the VS generator at that point). I think such a scenario is extremely unlikely, it's just object files and definitions here, but I just don't want us to be in the position of having the accept the limitations of the VS generator while also not getting the benefits of actually having it as something we support. Does that make sense?

@jschueller
Copy link
Collaborator Author

what do you mean the rest of it ? I think it has all the necessary bits @TLCFEM mentioned, except the exports macro problem which I adress by not changing the c lib name

@nbelakovski
Copy link
Contributor

So are you saying that with the latest changes it works with the VS generator?

@jschueller
Copy link
Collaborator Author

jschueller commented Jan 8, 2024

I could not test of course, but it should

@nbelakovski
Copy link
Contributor

Why couldn't you test?

@jschueller
Copy link
Collaborator Author

jschueller commented Jan 9, 2024

I dont have a proper windows machine, and I did not succeed in ci.

Mixing Fortran and C is not supported by VS generator
@nbelakovski
Copy link
Contributor

If you have enough space (~80GB) you can obtain an evaluation copy of windows for free from Microsoft: https://developer.microsoft.com/en-us/windows/downloads/virtual-machines/

It has an expiration date (I'm assuming they update it every couple of months), so you need to download a new image for future testing but at least it's free.

But this is what I meant by doing part of the work to support a feature and not doing the rest of it. Claiming that it should work is not sufficient, there needs to be some kind of documented evidence that it did work, even if that's just a comment from a developer saying "it worked on my machine." (ideally more than that, i.e. CI testing).

You seem to be going back and forth in this PR between trying to support VS generator and giving up. What is your goal with this PR?

@jschueller jschueller closed this Jan 10, 2024
@jschueller
Copy link
Collaborator Author

it is the best i can do

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