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.
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
build: Add CMake build #3621
base: main
Are you sure you want to change the base?
build: Add CMake build #3621
Changes from 96 commits
43daba2
f9c5cf0
f9b1ba7
4889de7
9eed8bc
67cdeee
faea30b
7f39bb3
96862bd
d9f674e
49c5d5b
b2669e5
18bf7f0
f76f68f
ba6c07d
76576db
2be2b75
e8126e7
52123fe
91461d5
87ef0aa
aecd006
4b23a6c
0a43a1c
e5becb7
fd5500d
9565839
0afe71f
617f58d
a9c23d7
3f1f5a1
6301f3e
d88134e
8c06b79
6b54a06
1852e08
dccaac0
c02053f
b222913
062b852
075e42e
555cdb2
754872d
9ef6a2b
161f1f9
ce13ea1
1541875
2af6d2e
1c2b3dd
028a323
1edfee8
6a12d3a
0f90e77
ce74248
31b53b5
a357bc7
aec2b7e
453739a
91515e0
c5de062
c6f17af
df254d1
c50fe07
2904d24
af2fe5c
999c71e
1527625
4d5e2b9
009d06e
e98b60b
12c5192
d27a929
306e24e
e8fc34c
b63b210
e05bfb3
3720a85
c870bbf
f8a71d7
2056b40
f61e9b4
906ecfb
3e0f6b4
322436c
60e0737
ee619ad
bf3abef
9a24f75
57eceb1
4a040d7
e61ca6f
38fc983
277ba5d
ee3ef6f
d46d202
23fbabb
9d5a1bf
cf06b01
ff0d592
fe3ba55
0bd4720
46ebe25
6d6f38c
e1bac2a
1d6e1dd
afe6fba
1639f8c
23621b5
6c1b216
4ad7eb2
bc460e6
023fb24
cc7fb73
9606c20
ded2773
63c048c
a1f54d3
7c072f8
76f6bbf
7534f7d
3fcc02a
9d4d22c
e5732bc
4c61830
f084cc0
ebfc26e
3dceff3
c364893
3f47bbe
7193663
e2d1198
f772e0f
e0e3003
ce96fca
ad18fc7
5c334f5
b6ce1d0
8ecd3cc
bdb5836
b8a0718
4dbb27e
2db8c1f
69b9091
021fd25
727f31b
f1aec67
5fefb87
1bbd7c6
3bdec35
050f940
9f10471
dfe4295
dcfa492
4debbaa
77aac55
04ec6f1
f6ac367
7da00eb
45dad8f
4b2aa27
e446882
8291252
0c87922
d3f25c5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Ideally the name of the runner wouldn't be hardcoded. Unfortunately, using
jobs.<job_id>.runs-on
shouldn't be available in the job-level env element. So, either, 1) stay like this and update later on, 2) keep the env, but move the assignment as a step (assigning with appending to the file with path GITHUB_ENV, maybe difficult with cross os runs), or 3) just place that extracted part in the key and restore key. Note that the cache key already contains the OS (runner os though), so it is only the topic of the cache that is needed. I may suggest of also specifying that it is for ccache too, if we ever have another cache in the workflow for another subject.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.
Todo (at the end): adjust with the final cmake version decided, or just use the version included in the runners. They are updated regularly, as new runner images are deployed weekly. It's not a compatibility problem, as the cmake_minimum_required sets the policies so that cmake will behave the same as the minimum version (by choosing the old behaviour for all behaviour changes made after that minimum version)
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.
With
-S
and-B
, it isn't needed to create the directory. That would make the single line show up more easily in the job log.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 see in the job logs that removing this caused a failure. It is created automatically, with this syntax, but that syntax only exists since cmake 3.13. So it fails at the call for configuring, but if the mkdir is added back, I expect it to immediately fail for another reason.
https://cmake.org/cmake/help/latest/manual/cmake.1.html#generate-a-project-buildsystem
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.
(It is related to the explicit installation of an old cmake version, that I think is not that useful, but only takes 1second in CI to run)
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.
Was it not working before this commit then?
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.
With that exact syntax and cmake version: probably. I remember suggesting the syntax a year or two ago, maybe in a PR from netelers branch, but I don't remember the part of the specific old version.
I know I read lots and lots for that, really trying to learn CMake (read 200+ page books for it, plus documentation), and was trying on my side (a year or more ago) to create cmakelists using only best practices, and to not rely on existing quirks/legacy bad practices, to see if I would come up with something. But first, I didn't know enough about CMake, nor specific enough about what builds really are, and lastly, didn't know enough of how GRASS works.
Now I understand way more, since I've been building it repeatedly since that time.
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.
cmake -B ... -S ...
is a CMake 3.13 addition. This is why the CI runner fails.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.
Shall we bump the req version to 3.16?!
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.
#3621 (comment)
Check warning on line 116 in .github/workflows/python-code-quality.yml