Cmake #934
Cmake #934
Conversation
|
@Iximiel great! I would suggest adding one thing to the todo list, which is possibility to compile plumed using Currently, in
In
It would be nice to have instead something like:
This would allow people using pip to have a fully working plumed package without the need to separately compile plumed. Regarding tests, I guess you will have to add a new job in the github action file. |
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## master #934 +/- ##
==========================================
+ Coverage 85.73% 85.79% +0.06%
==========================================
Files 600 600
Lines 53472 54727 +1255
==========================================
+ Hits 45843 46953 +1110
- Misses 7629 7774 +145
☔ View full report in Codecov by Sentry. |
src/analysis/CMakeLists.txt
Outdated
| @@ -0,0 +1,36 @@ | |||
| #automatically generated CMakeLists.txt, may not work! | |||
There was a problem hiding this comment.
@Iximiel are these file lists automatically generated? If so, is it possible not to include them in the repository?
Ideally, the build system will just use all the files in that directory. Can cmake do this?
There was a problem hiding this comment.
the base file is "automatically generated" by a homemade sh script (that I'm in doubt of including or not in the repository), but the ADDMODULEDEPENDENCIES macro arguments are them manually changed.
This because I wanted to reduce the "dependencies entanglement" and some circular dependency tree that exist if we use the list of dependencies in the "USE=" from the original Makefile (like core and tools).
I forgot to remove the lines "#automatically generated CMakeLists.txt, may not work!".
src/blas/def_external.h
Outdated
There was a problem hiding this comment.
@Iximiel these changes are only for readability right?
Please notice that these files are automatically generated
(see blas/import.sh and lapack/import.sh) from a gromacs repository. We did it some time ago, so you may need an older gromacs version (just do cd src/blas ; git log .).
Ideally, you could add these comments in the import.sh script, for consistency.
Another thing that I find very useful is to add comments with matching braces (see e.g. wrapper/Plumedh) because vim (yes :-)) can jump from one to the other.
There was a problem hiding this comment.
Yes, these changes in the source files are for readability.
I'm thinking that I should remove them, since they are "off topic" for this PR, and then add them in another PR with the import.sh updated as well
Another thing that I find very useful is to add comments with matching braces (see e.g. wrapper/Plumedh) because vim (yes :-)) can jump from one to the other.
You mean #endif /*__PLUMED_blas_def_internal_h */ instead of #endif //__PLUMED_blas_def_internal_h?
There was a problem hiding this comment.
I mean
#ifdef SOMETHING // {
#endif // }
There was a problem hiding this comment.
I mean in addition to your comment
|
If we keep this in the repository it will go out of sync very soon. Doesn’t
cmake allow to use wildcards?
Il giorno mer 10 mag 2023 alle 16:58 Daniele ***@***.***> ha
scritto:
***@***.**** commented on this pull request.
------------------------------
In src/analysis/CMakeLists.txt
<#934 (comment)>:
> @@ -0,0 +1,36 @@
+#automatically generated CMakeLists.txt, may not work!
the base file is "automatically generated" by a homemade sh script (that
I'm in doubt of including or not in the repository), but the
ADDMODULEDEPENDENCIES macro arguments are them manually changed.
This because I wanted to reduce the "dependencies entanglement" and some
circular dependency tree that exist if we use the list of dependencies in
the "USE=" from the original Makefile (like core and tools).
I forgot to remove the lines "#automatically generated CMakeLists.txt, may
not work!".
—
Reply to this email directly, view it on GitHub
<#934 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABBNCXVSQQKJM534Y2YZ5XDXFOUIFANCNFSM6AAAAAAX4SF6BY>
.
You are receiving this because you commented.Message ID:
***@***.***>
--
Sent from Gmail mobile
|
yes, we can use wildcard for getting the sources, but (source):
|
|
How much is the additional cost to do the check at every rebuild? Would it be possible to make automatically? Ideally, automatically when doing "make" (as it is now). Alternatively when configuring with "cmake" |
|
If you change a source file, the cost is the very like the same of automake, It needs to update the object and eventually relink it to it's archive or library. |
|
I see. Then can we perhaps have empty lists in the files stored on GitHub? If I understand correctly, the build system will then generate them when the users build. Sorry for being picky on this but I think it's a bad idea to store files that should be generated automatically. We have an exception ( However, to make sure that So, if you have to include those lists, I think somewhere on GitHub actions you have to check if they are consistent with the included file. And we will have to pay the price that whenever we add a new file we have to add it to the list or we receive an error. Still I think that the best solution is not to include those lists.
|
|
I see the point, having the list explicitly stored means you have to keep track of it and update the list in the right file each time you add one or more files. We can add a consistency check for the various Just to be clear: the |
Description
I set up the CMake build generator: it mimic the build process that calls
make install-buildin the src/lib directory, but not yet the install procedure of the original autoconf.It seems compiles
plumed,plumed-staticandplumed-runtimecorrectly, but I need to see if it works in a mac environmentIt can survive in parallel with autoconf and, for user like me used to vscode, may also be useful for some QoL integration with some IDEs.
This branch still need some love:
I think it should remain a draft until these are not completed
Target release
I think this should aim to be in the next release, even if it does not touch the actual code
Type of contribution
Tests