-
Notifications
You must be signed in to change notification settings - Fork 182
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
improve cmake build #51
Conversation
Thanks a lot. I like the spirit of these changes.
One issue with the working directory is that the tests also create files. So they will pollute the working directory. That I think is not expected when you use a build directory that ctest will create files outside of it.
…On Mon, Dec 30, 2019, at 12:34 PM, Michael Hirsch, Ph.D. wrote:
* use CMAKE_Fortran_MODULE_DIRECTORY to avoid introspecting directory
structure
* don't build in-source, use test WORKING_DIRECTORY instead
* use modern add_test() syntax to avoid introspecting project
directory structure
* use open(..., action=) to help indicate / safeguard intended file use
* don't have to accommodate CMake <= 2.4 circa 2006
closes #47 <#47>
You can view, comment on, or merge this pull request online at:
#51
Commit Summary
* improve cmake build
File Changes
* *M* .github/workflows/main.yml
<https://github.com/fortran-lang/stdlib/pull/51/files#diff-0> (6)
* *M* CMakeLists.txt
<https://github.com/fortran-lang/stdlib/pull/51/files#diff-1> (11)
* *M* src/stdlib_experimental_io.f90
<https://github.com/fortran-lang/stdlib/pull/51/files#diff-2> (18)
* *M* src/tests/ascii/CMakeLists.txt
<https://github.com/fortran-lang/stdlib/pull/51/files#diff-3> (5)
* *M* src/tests/loadtxt/CMakeLists.txt
<https://github.com/fortran-lang/stdlib/pull/51/files#diff-4> (11)
Patch Links:
* https://github.com/fortran-lang/stdlib/pull/51.patch
* https://github.com/fortran-lang/stdlib/pull/51.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#51?email_source=notifications&email_token=AAAFAWCAY6M2K7LQLIXZ77LQ3JEN3A5CNFSM4KBPQSB2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4IDMTDMQ>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAFAWGM5EJRSOQ7IJODS7TQ3JEN3ANCNFSM4KBPQSBQ>.
|
Thanks, I changed that test to accept an input directory argument, that will be in a build directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left two suggestions for indenting, the rest are just comments that are not a blocker. Otherwise +1 to merge!
@scivision please allow people with push access to modify your source branch (it's a check box somewhere). Then I can commit my suggestions above and merge this. Tons of other PRs depend on this. |
* use CMAKE_Fortran_MODULE_DIRECTORY to avoid introspecting directory structure * don't build in-source, use test WORKING_DIRECTORY instead * use modern add_test() syntax to avoid introspecting project directory structure * use open(..., action=) to help indicate / safeguard intended file use * don't have to accomodate CMake <= 2.4 circa 2006
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than @certik's indentation issues, this looks like a big improvement to me. I don't know who added calls to include_directories()
in and who approved the PR, but that is one of the ~5 CMake commands that should be blacklisted.
I added in, and then approved it. Don't know why, because it should not be used.
…On Mon, Dec 30, 2019, at 7:23 PM, zbeekman wrote:
***@***.**** approved this pull request.
Other than @certik <https://github.com/certik>'s indentation issues,
this looks like a big improvement to me. I don't know who added calls
to `include_directories()` in and who approved the PR, but that is one
of the ~5 CMake commands that should be blacklisted.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#51?email_source=notifications&email_token=AAAFAWFR4UJJAJZW37ZI6NLQ3KUK5A5CNFSM4KBPQSB2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCQNI3NI#pullrequestreview-337284533>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAFAWF5VWVGFMLIC5HBV3TQ3KUK5ANCNFSM4KBPQSBQ>.
|
Looks like this is ready to 🚢 |
closes #47