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

chore: some CMake cleanup #3564

Merged
merged 2 commits into from
Oct 28, 2024

Conversation

henryiii
Copy link
Contributor

Some things I noticed when trying something locally.

Signed-off-by: Henry Schreiner <[email protected]>
@@ -5,8 +5,6 @@
# The full license is in the file LICENSE, distributed with this software.

cmake_minimum_required(VERSION 3.16)
cmake_policy(SET CMP0025 NEW) # Introduced in cmake 3.0
cmake_policy(SET CMP0077 NEW) # Introduced in cmake 3.13
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting the minimum version of CMake sets all policies to NEW that were introduced on or before that value. (Also true if you set a version range, like 3.16...3.30)

@@ -37,7 +37,7 @@ Install development dependencies
.. code:: bash

micromamba create -n mamba -c conda-forge -f dev/environment-dev.yml
micromamba activate -n mamba
micromamba activate mamba
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no -n here.

find_package(reproc REQUIRED)
find_package(reproc++ REQUIRED)
find_package(Libsolv REQUIRED)
find_package(fmt CONFIG REQUIRED)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using CONFIG explicitly makes the error message better. I didn't go through every one to figure out whether they were CONFIG or MODULE, but these are some of the first ones hit, so error message is important for these.

set(libmamba_target mamba::libmamba-dyn)
else()
set(libmamba_target mamba::libmamba)
endif()

find_package(Python COMPONENTS Interpreter Development)
find_package(Python COMPONENTS Interpreter Development.Module)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default Development looks for the Embed target too, which isn't required and sometimes (like on manylinux) not available.

@jjerphan jjerphan added the release::enhancements For enhancements PRs or implementing features label Oct 23, 2024
Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @henryiii,

Thank you for bringing your expertise in build system and meta-build systems with this contribution!

Edit: I don't know whether the failures on Windows are related to that, and I can't debug it.

@henryiii
Copy link
Contributor Author

henryiii commented Oct 24, 2024

The PR after this one has the same windows error, and the one before this one claims to fix something on Windows (#3555).

@jjerphan
Copy link
Member

#3555 fixes an issue related to tests under Windows.

The error here is due to vcpkg which is used for the static builds of micromamba on Windows — this might be beyond our control.

@JohanMabille
Copy link
Member

This looks like a network issue, libxml2 is available at https://gitlab.gnome.org//GNOME/libxml2/-/archive/v2.11.9/libxml2-v2.11.9.tar.gz

Copy link
Member

@JohanMabille JohanMabille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the LibArchive vcpkg package does not provide a cmake config file

libmamba/CMakeLists.txt Outdated Show resolved Hide resolved
@JohanMabille
Copy link
Member

Thanks!

@JohanMabille JohanMabille merged commit 5bfdd90 into mamba-org:main Oct 28, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release::enhancements For enhancements PRs or implementing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants