maint: Cleanup CMake files and delete not compiled files #3667
Conversation
| ${LIBMAMBA_SOURCE_DIR}/util/url_manip.cpp | ||
| ${LIBMAMBA_SOURCE_DIR}/util/url.cpp |
There was a problem hiding this comment.
This is the correct sort order, and this is how it shows up in the tree list in VS Code, for example
| ${LIBMAMBA_INCLUDE_DIR}/mamba/util/variant_cmp.hpp | ||
| ${LIBMAMBA_INCLUDE_DIR}/mamba/util/weakening_map.hpp | ||
| # Implementation of version and matching specs | ||
| ${LIBMAMBA_INCLUDE_DIR}/mamba/specs/archive.hpp | ||
| ${LIBMAMBA_INCLUDE_DIR}/mamba/specs/authentication_info.hpp | ||
| ${LIBMAMBA_INCLUDE_DIR}/mamba/specs/build_number_spec.hpp | ||
| ${LIBMAMBA_INCLUDE_DIR}/mamba/specs/channel.hpp | ||
| ${LIBMAMBA_INCLUDE_DIR}/mamba/specs/chimera_string_spec.hpp |
There was a problem hiding this comment.
This is an example of missing line. Everything will work except that VS Code will give errors when you open this file - it annoyed me, so I decided to create this PR
| @@ -1,182 +0,0 @@ | |||
| // Copyright (c) 2023, QuantStack and Mamba Contributors | |||
There was a problem hiding this comment.
This file is not included by anyone, and both it and .cpp file are not mentioned in CMake, so I think it's safe to remove it.
Please, make sure I'm right
micromamba/src/microserver.cpp
Outdated
| @@ -1,414 +0,0 @@ | |||
| // Copyright (c) Alex Movsisyan | |||
There was a problem hiding this comment.
This file and server.cpp are not mentioned in CMake at all, so I think it's safe to remove it.
Please, make sure I'm right.
| @@ -11,6 +11,7 @@ add_executable( | |||
| src/main.cpp | |||
| src/msvc_catch_string_view.cpp | |||
| src/pool_data.cpp | |||
| src/pool_data.hpp | |||
There was a problem hiding this comment.
There is no separation for source and header files here, so we should mention this header file as well.
There was a problem hiding this comment.
This header file is not required strictly here, but for proper IDE integration, is that right?
There was a problem hiding this comment.
Yes.
And it also makes it consistent (all source/header files are present in CMake files).
mathbunnyru
changed the title
mathbunnyru
changed the title
| @@ -11,6 +11,7 @@ add_executable( | |||
| src/main.cpp | |||
| src/msvc_catch_string_view.cpp | |||
| src/pool_data.cpp | |||
| src/pool_data.hpp | |||
There was a problem hiding this comment.
This header file is not required strictly here, but for proper IDE integration, is that right?
Thanks! Replied to your comment. I agree that having coverage would be nice, but I don't think it would help here. |
|
I agree with you and I meant that adding code coverage is an addiction to dead code elimination. 🙂 |
|
@jjerphan I think this is ready to be merged as well 🙂 |
|
For this one, I would wait for another review of the team. |
micromamba/src/server.cpp
Outdated
There was a problem hiding this comment.
This file is still used in micromamba's CMakeLists.txt, and it includes microserver.cpp, so I don't think we should remove them for now.
There was a problem hiding this comment.
@JohanMabille thanks!
I didn't notice that. Returned these files back.
There was a problem hiding this comment.
Just as a note: these files are not compiled in any way right now, and their compilation is significantly broken.
There was a problem hiding this comment.
So, I think, if we want to keep these files, we need to fix the compilation and add it to the CI.
Otherwise we'll just have broken files in the repo, which no one is able to use easily.
There was a problem hiding this comment.
Created an issue: #3677
There was a problem hiding this comment.
@mathbunnyru I agree; IIRC we initially wanted to remove these files and then realized we need them for another project (I think Quetz). We kept them for reference because we need to refactor them (ok, maybe it will be a full rewrite, but we still need to keep them for now), but did not have the bandwidth to do it. We'll probably tackle this in 2025.
|
Thanks for the cleanup! |
I did the following: