Giter Site home page Giter Site logo

Comments (9)

bbannier avatar bbannier commented on August 25, 2024 1

Thanks! I am certainly open to helping with this (e.g., by looking over PRs), or even contributing an implementation myself once your new API has become clearer. Feel free to ping me.

from kondo.

tbillington avatar tbillington commented on August 25, 2024

Oh I didn't know about CMakeCache.txt.

I'm working on "v2" that greatly increases the flexibility of the internal logic, here is an example. I think it would be possible to support searching subdirectories for CMakeCache.txt.

Do you think this API would fully support that use-case? Ideally we could determine the directory from CMakeLists but it's not a common format that's easy to parse.

fn is_artifact(&self, path: &Path) -> bool {
path.is_dir()
&& path
.file_name()
.is_some_and(|f| PATHS.iter().any(|p| *p == f))
}
fn artifacts(&self, root_dir: &Path) -> Vec<PathBuf> {
filter_exists(root_dir, &PATHS).collect()
}

from kondo.

bbannier avatar bbannier commented on August 25, 2024

Do you think this API would fully support that use-case? Ideally we could determine the directory from CMakeLists but it's not a common format that's easy to parse.

A top-level CMakeLists.txt could be used to determine the project name by parsing the optional project command, see https://cmake.org/cmake/help/latest/command/project.html. The top-level file should have at most one of these (though it could also be absent).

As for artifacts, the build dir is not set in CMakeLists.txt, but instead determined by how the user invokes cmake. The typical workflow is to create an arbitrarily named build directory somewhere (could be as a subdirectory of the project, but might also be a directory outside of the project directory, or even the project directory itself), and then invoke cmake from that directory, e.g.,

# Minimal CMake config.
touch CMakeLists.txt

# All of these are valid ways to configure the project, even simultaneously.
(mkdir build && cd build && cmake ..)                    # 1. Currently supported.
(mkdir _ && cd _ && cmake ..)                            # 2. Unsupported.
(mkdir build/config1 && cd build/config1 && cmake ../..) # 3. Unsupported.
(mkdir /tmp/build && cd /tmp/build && cmake /project/)   # 4. Unsupported, assumes project tree in `/project/`.
cmake .                                                  # 5. Unsupported.

(1) and (2) can be supported by looking for a subdirectory with CMakeCache.txt, but to support (3) we'd need to crawl the full project tree.

This approach breaks down for (4) where CMakeLists.txt and build directory with CMakeCache.txt have no relationship in the file system (though we could go from build directory to source directory by inspecting CMAKE_HOME_DIRECTORY in CMakeCache.txt).

One can detect (5) (CMakeCache.txt right next to CMakeLists.txt), but in this case it is impossible to decide what was an input and what is an artifact. It would probably make sense to detect this, but not perform any cleanups in this case.

The current interface sketch for Project could in principle support this, but potentially makes e.g., detection of root_dir for e.g., (4) awkward. It is passed in as an argument to artifacts, but not available for is_artifact. If the API would guarantee that e.g., is_project would always be called first one could store it away and make use of it. More generally, nothing in the trait promises that is_artifact is only called for files under root_dir, and from an API user perspective I'd rather only implement artifacts (directory with CMakeCache.txt and all files in it).

For all these reasons a better implementation might be to instead in a first implementation treat the build directory (directory with CMakeCache.txt) as root_dir and only ever look for CMakeLists.txt for the project name or to avoid removing files in the actual CMake project directory for (5) (in treating this there might be some overlap with CMakeCache.txt in a different kind of project, say CMakeCache.txt next to Cargo.toml, e.g., by having two CMake impls for Project, one for source trees which performs no cleanup and another one for CMake build trees which cleans up; this way a "good impl" for #111 would take care of (5) automatically).

from kondo.

tbillington avatar tbillington commented on August 25, 2024

Thank you for the detailed breakdown, I haven't personally user CMake before so this is helpful.

I've fixed the naming of is_artifact -> is_root_artifact and artifacts -> root_artifacts to represent what they were actually doing.


If the API would guarantee that e.g., is_project would always be called first one could store it away and make use of it.

I've added that assumption to the docstring of the methods, thanks for pointing that out.

There may be a way to enforce this at a type level so it's impossible to not have a found project before you check artifacts etc. For example is_project would be something that returns a "scoped" path/project, similar to Project in the current version.

kondo/kondo-lib/src/lib.rs

Lines 124 to 127 in 415cd33

pub struct Project {
pub project_type: ProjectType,
pub path: path::PathBuf,
}


I'm still thinking over your last couple paragraphs :)

from kondo.

tbillington avatar tbillington commented on August 25, 2024

I noticed while reading the project docs that PROJECT_BINARY_DIR exists.

Is this commonly used? Perhaps the binary path could be retrieved

from kondo.

tbillington avatar tbillington commented on August 25, 2024

Option 1 ✅

(mkdir build && cd build && cmake ..)                    # 1. Currently supported.

Already supported.

Option 2 ✅

(mkdir _ && cd _ && cmake ..)                            # 2. Unsupported.

This shouldn't be a problem to implement. Requires recursive searching for CMakeCache.txt.

Option 3 👍

(mkdir build/config1 && cd build/config1 && cmake ../..) # 3. Unsupported.

It seems based on your 3rd option that only supporting artifacts at the root of the project is insufficient. I'll have a think how to address that.

Perhaps Project::root_artifacts should be able to return the top level of the artifact directory even if it's not in the project root. In this example it would include build/config.

Option 4 ⚠️

(mkdir /tmp/build && cd /tmp/build && cmake /project/)   # 4. Unsupported, assumes project tree in `/project/`.

This is an interesting one, until now nothing in kondo has required "out of tree" knowledge/logic. The possible solutions I can think of off the top of my head are not attractive. I'm tempted to just say this isn't supported for now. Can you think of any other project software like cmake etc that might have behave similarly? I wonder how common this pattern is.

Option 5 ⚠️

cmake .                                                  # 5. Unsupported.

I had a look at the output of CMakeCache.txt, there is some indirection but eventually I found a list of the files the in the generated CMakeFiles/Project.dir/cmake_clean. So we could either shell out to cmake to run it's (Makefile's?) generated clean (if it exists) or try and parse that file to find the files to delete ourselves.

Do you think the file list defined here is reliable? Here's an example generated file from me completing the cmake tutorial.

file(REMOVE_RECURSE
  "CMakeFiles/Tutorial.dir/tutorial.cxx.o"
  "CMakeFiles/Tutorial.dir/tutorial.cxx.o.d"
  "Tutorial"
  "Tutorial.pdb"
)

# Per-language clean rules from dependency scanning.
foreach(lang CXX)
  include(CMakeFiles/Tutorial.dir/cmake_clean_${lang}.cmake OPTIONAL)
endforeach()

from kondo.

tbillington avatar tbillington commented on August 25, 2024

Do you know if there is a preference/rough order of which options tends to be most common for cmake projects?

from kondo.

tbillington avatar tbillington commented on August 25, 2024

I have added option 1 support for the rework in 9b48edb.

from kondo.

bbannier avatar bbannier commented on August 25, 2024

I noticed while reading the project docs that PROJECT_BINARY_DIR exists.

Is this commonly used? Perhaps the binary path could be retrieved

This is typically use to construct paths to artifacts (e.g., to reference generated sources or headers). A similar variable is CMAKE_BINARY_DIR. Variables with PROJECT_ prefix (instead of CMAKE_) are used of one wants to support nested projects.

I have never worked on a project which sets these as opposed to read the values set by CMake.

Do you know if there is a preference/rough order of which options tends to be most common for cmake projects?

I'd say that building in a subdirectory is pretty typical (variations of (1) or (2)), but I also know people who e.g., keep their build directories on a different partition, i.e., (4). For quick and dirty stuff people also use (3).

Re: (4) (external build directory):

The possible solutions I can think of off the top of my head are not attractive. I'm tempted to just say this isn't supported for now.

This would be really unfortunate. I am not exactly sure what you tried, but as far as kondo is concerned the directory with CMakeLists.txt isn't even interesting and it would be sufficient to only work on directories with CMakeCache.txt. This is what I meant in #120 (comment):

  • discover CMake build directories by the presence of a toplevel file CMakeCache.txt (i.e., CMake build directory is "project directory)
  • they can be cleaned up if that would not conflict with other project setups (#111)
    • potential conflict with e.g., Rust setup if Cargo.toml is present next to CMakeCache.txt
    • to avoid running into (5) (CMake build in source directory): create another flavor of CMake project which triggers on presence of CMakeLists.txt

Re: (5) (build in source directory):

I had a look at the output of CMakeCache.txt, there is some indirection but eventually I found a list of the files the in the generated CMakeFiles/Project.dir/cmake_clean. So we could either #122 to cmake to run it's (Makefile's?) generated clean (if it exists) or try and parse that file to find the files to delete ourselves.

Do you think the file list defined here is reliable? Here's an example generated file from me completing the cmake tutorial

The presence of that file seems to depend on the build system used (CMake supports generating for an open-ended zoo of build systems), e.g., I do not see it for the Ninja generator. As far as I know there is also no standard CMake command to clean up the build directory including the build directory itself (equivalent of e.g., cargo clean). It would also leave the question of whether the build directory should be removed or not since it would never appear in any of these listings (it likely was created by the user in possibly conflicting ways). With that it seems impossible to have CMake take care of cleaning up the artifacts (which is what makes kondo useful).

Another issue with defaulting to the build tools for #122 is that it would require the build tool to be present on the system which might not be the case. CMake projects might reconfigure when running cmake again which could run into issues of e.g., the CMake version changed, or files are outdated; each of these are not unlikely to fail.

Can you think of any other project software like cmake etc that might have behave similarly? I wonder how common this pattern is.

Autotools would behave similarly.

from kondo.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.