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

Feature/conan2 #863

Merged
merged 8 commits into from
Dec 10, 2024
Merged

Feature/conan2 #863

merged 8 commits into from
Dec 10, 2024

Conversation

schaubh
Copy link
Contributor

@schaubh schaubh commented Nov 28, 2024

  • Tickets addressed: bsk-860
  • Review: By commit
  • Merge strategy: Merge (no squash)

Description

The Basilisk build system was not compatible with conan 2.x. The conan project now is no longer updating conan 1.x recipes.

Verification

The build system works again with conan 2.x installed. All the CI tests passed. Wheels were built and installed with the expected results.

Documentation

The build documentation was updated to say that the latest version of conan can be installed. The release notes were updated. The known issue file has a comment that because conan 2.x uses a new .conan2 folder for the dependencies, the first time this BSK commit is built the conan dependencies are downloaded and installed. This is a one-time action.

Future work

Hopefully conan 3.x won't break conan 2.x scripts...

@schaubh schaubh added enhancement New feature or request build Build system or compilation enhancement labels Nov 28, 2024
@schaubh schaubh requested a review from Mark2000 November 28, 2024 22:15
@schaubh schaubh self-assigned this Nov 28, 2024
@schaubh schaubh requested a review from a team as a code owner November 28, 2024 22:15
@schaubh schaubh linked an issue Nov 28, 2024 that may be closed by this pull request
@schaubh schaubh marked this pull request as draft November 28, 2024 22:22
@schaubh schaubh force-pushed the feature/conan2 branch 2 times, most recently from b67b165 to 47ee8da Compare November 28, 2024 23:12
@schaubh
Copy link
Contributor Author

schaubh commented Nov 29, 2024

@dpad , thank for the help creating a build script earlier this summer that worked with conan 2.x. I was able to now upgrade our latest conanfile.py to use the existing build system that supports creating wheels and IDE projects to support conan 2.x. I'm inviting you to look at this branch as well in case you have feedback. I'm making BSK require conan>2.0.5 at this point and dropping support for conan 1.x. I didn't see a need for continued conan 1 support as conan themselves said they are no longer updating conan 1.x packages.

@sassy-asjp, you mentioned the urgent need for conan 2 support, so I'm inviting you as well to take a look.

My next step is to break up the requirements.txt file into a requirements file for developers to build basilisk, and a requirements file to run the core BSK capability (i.e. if installed via wheel.)

conanfile.py Outdated Show resolved Hide resolved
@schaubh schaubh marked this pull request as ready for review November 29, 2024 15:45
@schaubh schaubh requested a review from sassy-asjp November 29, 2024 16:14
Copy link
Contributor

@sassy-asjp sassy-asjp left a comment

Choose a reason for hiding this comment

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

Thanks so much for doing this. I had a couple comments but I think it's fine to merge as is.

src/utilities/bskLargeData.py Outdated Show resolved Hide resolved
supportData/EphemerisData/MAR033_2000-2025.bsp Outdated Show resolved Hide resolved
conanfile.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Mark2000 Mark2000 left a comment

Choose a reason for hiding this comment

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

Looks good, documentation changes seem clear to me.

Only use Xcode default if the user says to not build the project and doesn't provide a generator string.
In Xcode 16 there is an issue where Xcode is not finding "python.h".  Appears to be a regression.
This change in compiling BSK does not impact users using "python conanfile.py", the terminal
output is actually easier to read when compiled with makefile, and there are no issues with not
finding "python.h".
swig 4.2.1 or great is ok with Basilisk
This issue has been fixed in the current version of conan.
@schaubh schaubh merged commit 0eedde4 into develop Dec 10, 2024
9 checks passed
@schaubh schaubh deleted the feature/conan2 branch December 10, 2024 03:41
try:
subprocess.check_output(["conan", "profile", "new", "default", "--detect"])
# XXX: This fixes a linker issue due to the dual C++ ABI.
subprocess.check_output(["conan", "profile", "update", "settings.compiler.libcxx=libstdc++11", "default"])
Copy link
Contributor

Choose a reason for hiding this comment

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

@schaubh (This is only minor) Just FYI, this command does not work on Conan2, so this will always fail. There is no conan profile update anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Howdy @dpad , thanks for the feedback. The Linux CI builds seem to run fine. Are you recommending I remove this Linux specific conan command? It is actually specifying c++11 which doesn't look right to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm not mistaken, with Conan 2, this line will always fail and go straight into the exception, which does nothing. So this line is doing nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make a branch to remove this dead code. Thanks for the feedback, especially on Linux systems.

Copy link
Contributor

Choose a reason for hiding this comment

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

@schaubh The c++11 actually refers to the breaking change specific to gcc libstdc++ ABI implemented for c++11. https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_dual_abi.html

FWIW, I just tried conan profile detect on a Linux VM (Ubuntu 24.04) and it automatically chooses libstdc++11, so even if the command worked, it would be redundant.

@schaubh
Copy link
Contributor Author

schaubh commented Dec 11, 2024

Good to know @sassy-asjp , thanks for sharing this info. In my PR #872 the CI build worked fine across all the Linux versions that we test against.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Build system or compilation enhancement enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

conan 2.x compatibility
4 participants