Skip to content
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

De-couple root Makefile from root CMakeLists.txt #611

Merged
merged 2 commits into from
Dec 21, 2023

Conversation

mmuetzel
Copy link
Contributor

@mmuetzel mmuetzel commented Dec 21, 2023

Maybe, that is not a supported use-case. But I'm often using the same source tree to test building with the root Makefile and with the root CMakeLists.txt. However, the purge target of the root Makefile also removes the build tree of the root CMakeLists.txt. That is annoying sometimes.

I'm working around that by using a different folder to ./build when building with the root CMakeLists.txt. So, I don't mind if you would prefer to not accept this change. I could just continue with that "workaround".

The proposed change would remove that the root Makefile interacts with the (default) folder for the build tree of the root CMakeLists.txt. Instead, it would introduce a (new) target "purge" to the root CMakeLists.txt that would remove all files from its own build tree.
That new target doesn't do much more than executing rm -rf * (or whatever command that corresponds to on a platform without POSIX-compatible binutils). A developer could probably also just use that command instead of make purge. So, it would also be fine if just the second part (of removing the interaction of the root Makefile with the build tree of the root CMakeLists.txt) would be accepted.

Run `cmake --build . --target purge` (or `make purge` if CMake uses a
Makefile generator) to remove all files in the build tree.

Remove a target of the same name from the build rules of Mongoose to
avoid a target name conflict.
@DrTimothyAldenDavis
Copy link
Owner

Sure, we could figure out something that works. It would be nice to have several options:

  1. remove all SuiteSparse/Package/build/, any compiled MATLAB mexFunctions, and any temporary files in SuiteSparse/Package/Tcov folders (these are my non-cmake-based test coverage setups; they only work on Linux). I should also remove any temporary files in SuiteSparse/Package/Doc/, typically created by pdflatex. The default "make" in most Doc folders does this already, except for a few packages so it's a bit inconsistent. Mongoose is a little unusual; it also has tests that can download matrices directly into the source tree. I revised it so that the ctests download them into the build/Matrix folder instead, which is then deleted when the Mongoose/build or SuiteSparse/build/Mongoose folders are deleted.
  2. remove just SuiteSparse/build/*
  3. do all of the above

My "make purge" is my own usage; the phrase is kind of unusual. We could just remove it or repurpose it. The more common practice is "make clean" or "make distclean" which is why I have those as synonyms in each Makefile.

"make distclean" is more a common usage, and it removes all files not in the original distribution. So it's more like option (3) above. It should clean everything.

What if we gave (1) and (2) their own names?

Inside a cmake script, instead of using "rm -rf", the better thing in cmake would be to use

file ( REMOVE  files )
file ( REMOVE_RECURSE files_and_directories )

https://cmake.org/cmake/help/latest/command/file.html#remove

That would be the portable way to delete files. The only downside to using cmake to delete files is that removing build/* deletes the constructed cmake files with the target itself.

The Mongoose CMakeLists.txt uses rm -f and rm -rf but that should be either removed from the CMakeLists.txt

@mmuetzel
Copy link
Contributor Author

The clean target is always being created by CMake. It deletes the build artifact (object files, libraries, ...). But it doesn't delete the CMake cache or the generated Makefile (or whatever the selected generator is).
The proposed purge target would delete everything in the build tree. It wouldn't touch anything that might be created in the source tree (what a distclean target usually does).

file commands cannot be used in a target. cmake -E rm -rf does not necessarily call rm -rf. In fact, rm -rf are just arguments to cmake -E that "happen" to resemble that command a lot. Actually, CMake will run the commands that would do the "right thing" for the platform it runs on. So, doing that should be as portable as one can be.

@mmuetzel
Copy link
Contributor Author

mmuetzel commented Dec 21, 2023

Wrt the above points:
Re 1: Call make purge with the root Makefile.
Re 2: Call make purge with the Makefile generated by CMake.
Re 3: Imho, the two build systems shouldn't interact in that way.

@DrTimothyAldenDavis
Copy link
Owner

Re 1: Call make purge with the root Makefile.
Re 2: Call make purge with the Makefile generated by CMake.
Re 3: Imho, the two build systems shouldn't interact in that way.

OK. I can do that. Will this PR do both (1) and (2) then?

@mmuetzel
Copy link
Contributor Author

OK. I can do that. Will this PR do both (1) and (2) then?

I didn't touch much in the root Makefile. If there are parts of (1) that the root Makefile doesn't do currently, it won't do it after this change.
This change only adds a purge target to the CMake build rules (2). That purge target removes all files from the build directory that CMake is using. It doesn't do anything with files in the source directory.

If you find it confusing that there are two purge targets that do different things for the two build systems, we could also use a different name for the new target. Which name would you suggest?

@mmuetzel
Copy link
Contributor Author

To clarify, I'd prefer if running make purge with the root Makefile would not touch anything in the CMake build folder.
So, only the second commit would resolve that issue.
The first commit is only there to add something to the CMake build system that does what the root Makefile did before. But that rule should not be used by the root Makefile. Otherwise, the whole PR would be pointless.

@DrTimothyAldenDavis
Copy link
Owner

That won't be confusing. This is all fine.

And if I want a single command to clean everything, I can always do that locally in an alias or shell script that's completely outside the SuiteSparse repo. Just for my own use.

@DrTimothyAldenDavis DrTimothyAldenDavis merged commit 2f0ee55 into DrTimothyAldenDavis:dev2 Dec 21, 2023
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants