Merged
Conversation
Member
|
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>.
|
Member
Author
|
Thanks, I changed that test to accept an input directory argument, that will be in a build directory |
milancurcic
approved these changes
Dec 30, 2019
Merged
certik
requested changes
Dec 30, 2019
Member
certik
left a comment
certik
left a comment
There was a problem hiding this comment.
I left two suggestions for indenting, the rest are just comments that are not a blocker. Otherwise +1 to merge!
Member
|
@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
zbeekman
reviewed
Dec 31, 2019
zbeekman
approved these changes
Dec 31, 2019
Member
zbeekman
left a comment
zbeekman
left a comment
There was a problem hiding this comment.
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.
Member
|
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>.
|
zbeekman
reviewed
Dec 31, 2019
zbeekman
reviewed
Dec 31, 2019
Member
|
Looks like this is ready to 🚢 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #47