Skip to content

Add TRX support to tckconvert#3265

Open
mattcieslak wants to merge 2 commits intoMRtrix3:devfrom
PennLINC:add-trx-dev
Open

Add TRX support to tckconvert#3265
mattcieslak wants to merge 2 commits intoMRtrix3:devfrom
PennLINC:add-trx-dev

Conversation

@mattcieslak
Copy link

This adds picks up where #3262 left off, adding TRX support to tckconvert, updates the documentation and adds tests. It depends on MRtrix3/test_data#6 to pass the tests.

I haven't added trx-cpp to any of the builds - I was hoping for some input on how would be most convenient to do so.

Thanks in advance!

@daljit46
Copy link
Member

Currently, we handle dependencies through CMake's FetchContent with optional flags to use a system-provided version of the given library. I think the same approach should work fine with trx-cpp. One issue would be that trx-cpp depends on libzip and expects that to be installed on the user's system. So I see two options here: either mandate the installation of libzip for building the project (as we do for Qt) or we also fetch libzip via FetchContent when trx-cpp support is required. @jdtournier @Lestropie any thoughts?

@MRtrix3/mrtrix3-devs as an aside, I think this issue highlights some of the limitations of using FetchContent for dependency handling, as dealing with transitive dependencies is not straightforward and we may want to consider adopting a package manager like vcpkg.

@jdtournier
Copy link
Member

I think we should keep it as simple as possible for now, and document the expectations sufficiently for other developers. As long as regular users can use the binaries we produce, they won't care either way. It's a relatively small subset of people who might benefit from transitioning to a more complex package manager, and few of them will be particularly upset if they need to install other packages manually. Realistically, I don't think it's worth investing much time in finessing the developer experience...

The bigger issue is whether we need to bundle these additional dependencies in the binary packages (which I assume we should?).

@Lestropie
Copy link
Member

I'm not perturbed about specifically inheriting a libzip dependency given I'd like to support the .npz format (#2682) that I think would itself require libzip. Not to say that transitive dependency isn't a problem, just that this particular one doesn't scream problematic to me. It's pretty common as a source compilation dependency and stable as an API, and the runtime dependency will be ubiquitous, so I'd just mandate it at system level.

@daljit46
Copy link
Member

daljit46 commented Jan 30, 2026

@mattcieslak Given the comments above, I think adding something along the lines of:

if(MRTRIX_USE_SYSTEM_TRXCPP)
    find_package(trx-cpp CONFIG REQUIRED)
else()
    set(trxcpp_url "https://github.com/tee-ar-ex/trx-cpp")
    FetchContent_Declare(
        trx-cpp
        DOWNLOAD_EXTRACT_TIMESTAMP ON
        GIT_REPOSITORY ${trxcpp_url}
        GIT_TAG ea9ebcbf4b08e047e6493eb8077f789cdfac36b7
    )
    FetchContent_MakeAvailable(trx-cpp)
endif()

in cmake/Dependencies.cmake should do the trick. We'll also need to update our build instructions and CI workflows for the installation of libzip.

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.

4 participants