Skip to content

Code reorganization #2829

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

Merged
merged 1 commit into from
Mar 25, 2025
Merged

Code reorganization #2829

merged 1 commit into from
Mar 25, 2025

Conversation

JohanMabille
Copy link
Member

Checklist

  • The title and commit message(s) are descriptive.
  • Small commits made to fix your PR have been squashed to avoid history pollution.
  • Tests have been added for new features or bug fixes.
  • API of new functions and classes are documented.

Description

@JohanMabille JohanMabille merged commit 4a91e4e into xtensor-stack:master Mar 25, 2025
16 checks passed
@JohanMabille JohanMabille deleted the reorg branch March 25, 2025 05:24
@jtramm
Copy link

jtramm commented Mar 28, 2025

This PR seems to change the location of all the headers, meaning:

  1. All downstream users will need to update their includes to account for the new header paths
  2. Users that pull in system libraries of 0.25.0 vs. a cmake submodule 0.26.0 will now conflict, likely causing a lot of issues with downstream CMake builds etc.
  3. All xtensor documentation is now out of date as it uses the original header paths.

Is there a strong motivation for this reorg, as the downsides seem extreme compared to the upgrade to folder tidiness? Or perhaps this wasn't meant to go into production?

@acmorrow
Copy link

acmorrow commented Apr 1, 2025

@jtramm - Yeah this just caused me a bit of a headache. Now I have software with old style xtensor includes. If I check in a change to move to the new ones, I'll break builds of my software on platforms that have the older version of xtensor installed.

In order to continue building, I expect I'll need to do some sort of

#if XTENSOR_VERSION_MAJOR >=0 && XTENSOR_VERSION_MINOR >= 26
#include <xtensor/containers/xarray.hpp>
#else
#include <xtensor/xarray.hpp>
#endif

Which doesn't feel great. Or I guess I can write my own wrapper headers that do that.

Typically, if I was undertaking this sort of a reorg in a project I manage, I'd go about it by leaving the old headers around for a few cycles with a deprecation warning and having them include the relocated headers. That way old code isn't broken but new code gets nudged into converting to the new style.

On the other hand, the major version here is 0, so, fair to do this I guess, from a purely semver perspective.

@acmorrow
Copy link

acmorrow commented Apr 1, 2025

Oh, wait, I actually can't even do that, because the header defining XTENSOR_VERSION_MAJOR and friends got moved too, so I have no stable header I can include to determine the version. On to __has_include I guess.

@danielcjacobs
Copy link

Yeah, not sure why this MR needed to be done. Just wanted to upgrade to 0.26.0 for compatibility with newer clang versions and now everything is broken.

JohanMabille added a commit to xtensor-stack/xtensor-python that referenced this pull request Apr 14, 2025
Brings xtensor-python up to date with latest xtensor release (0.26.0),
where xtensor-stack/xtensor#2829 reorganized the
include directory.

---------

Co-authored-by: Daniel Jacobs <[email protected]>
Co-authored-by: Johan Mabille <[email protected]>
@naegelejd
Copy link

It's pretty wild this was merged without any response to the concerns above. Obviously you can do what you want, but wow, this is a major breaking change that isn't documented anywhere! The changelog just says "Code reorganization" which is not helpful at all.

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.

5 participants