CMake: misc

[jschueller]

• 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]

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]

just tyding things up

[nbelakovski]

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.

[zaikunzhang]

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

[jschueller]

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

[nbelakovski]

@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]

done

[nbelakovski]

@zaikunzhang I believe this PR is good to go.

[zaikunzhang]

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]

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]

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

[nbelakovski]

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 https://github.com/fortran-lang/setup-fortran/issues/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]

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]

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

[jschueller]

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

[nbelakovski]

@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]

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]

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]

I found other links claiming that VS/msbuild does not support more than a single language at once:

• https://stackoverflow.com/questions/5434109/why-are-visual-studio-projects-restricted-to-a-single-language
• https://cmake.cmake.narkive.com/0UQygsqi/mixed-fortran-and-c-projects-in-visual-studio-with-intel-fortran-compiler

[nbelakovski]

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]

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]

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

[jschueller]

I could not test of course, but it should

[nbelakovski]

Why couldn't you test?

[jschueller]

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

[nbelakovski]

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]

it is the best i can do