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

Renaming of HDPS to ManiVault #395

Merged
merged 18 commits into from
Oct 11, 2023
Merged

Renaming of HDPS to ManiVault #395

merged 18 commits into from
Oct 11, 2023

Conversation

JulianThijssen
Copy link
Contributor

@JulianThijssen JulianThijssen commented Sep 20, 2023

In this pull request:

  • Implicit renaming of hdps namespace to mv
  • Update of version number to 1.0.0
  • Renaming of the application name to ManiVault Studio.exe
  • Renaming of core build targets, published libraries (MV_Private, MV_Public, subject to change? based on our last discussion)
  • Renaming of HDPS_INSTALL_DIR to MV_INSTALL_DIR (mostly to not break anything, but for new users it basically shouldn't do anything anymore)
  • Renaming of HDPS_USE_AVX to MV_USE_AVX
  • Renaming of OSX bundle name and gui identifier

This pull request first included an overhaul of the CMake building to use config files, which allow us to link to manivault using find_package(ManiVault), target_link_libraries(ManiVault::Core). Instead of the 10 lines of code to establish the proper library name on every platform. Added benefit being the ability to include defines, see namespaces section. After discussion, I have separated out the config file update for post-release.

Namespaces: Unfortunately without a config file, there is no nice way to embed a #define mv hdps in the public library. The core namespace will stay hdps temporarily. For now, we can iteratively add target_compile_definitions(${PROJECT} PRIVATE mv=hdps) to every plugin cmake, and find/replace its namespace to mv. The define will transform each occurance of mv to hdps.
In this way, no plugin is automatically broken by merging this pull request, and when we finish with replacing their use of the hdps namespace, we can simply find/replace hpds to mv in the core, and remove the target_compile_definitions from all plugin cmakes.

@JulianThijssen JulianThijssen added core amendment A change to the code to improve the quality labels Sep 20, 2023
@thoellt
Copy link
Collaborator

thoellt commented Sep 25, 2023

The built application does not work for me on the Mac. It starts up and generates the following error:

image

Another smaller issue, The Xcode project is still called hdps-main.xcodeproj defined in line 5 of the main CMAKE file, maybe this can be changed to manivault* with little consequences?

@bldrvnlw
Copy link
Contributor

In the conanfile.py the HDPS_INSTALL_DIR should change to MV_INSTALL_DIR.

Also in the conan file the package name for the artifactory is still hdps-core. Leave this for the moment. I'll change this to mv-core in a separate commit as this needs to be coordinated with changes to the rulesupport login (that is used to sync feature branches of plugins with the corresponding core).

@alxvth
Copy link
Contributor

alxvth commented Sep 26, 2023

This is also something to keep in mind: Every plugin uses HDPS_INSTALL_DIR in their CMake file to find ManiVault.

As far as I know the generated HdpsCoreTargets.cmake/ManiVaultTargets.cmake do not work currently. They could at some point be used to find ManiVault's link and headers libraries easier with find_package(ManiVault 1.0 COMPONENTS Core PointData ClustreData REQUIRED) instead of the current approach of using the HDPS_INSTALL_DIR.
For now, we might as well delete the ManiVaultTargets.cmake generation - it might only be confusing for users who'd try using it.

@JulianThijssen
Copy link
Contributor Author

@alxvth
The plugins only use HDPS_INSTALL_DIR currently to set their initial INSTALL_DIR, however, if this environment variable is not set they will have to set the INSTALL_DIR themselves, which is the intended behaviour. Therefore, renaming the variable here does not inherently break the plugins. Ideally, we totally get rid of it, but changing it to MV_INSTALL_DIR for now at least keeps the core working while we phase it out from the plugins.

Indeed, I agree targets and config should be used in future (see my PR description), but I have separated this change out as it is not absolutely required for the release and after discussion we'd like to limit such changes as much as possible.

@alxvth
Copy link
Contributor

alxvth commented Sep 26, 2023

Yes, I understand. My proposal would just be to comment out or delete

write_basic_package_version_file(
    ...
)

configure_package_config_file(
    ...
)

install(FILES
      "${CMAKE_CURRENT_BINARY_DIR}/HdpsCoreConfig.cmake"
      "${CMAKE_CURRENT_BINARY_DIR}/HdpsCoreConfigVersion.cmake"
      ...
)

for the time being since their output is not used anywhere, doesn't work and might just confuse developers.

(If in this PR or another I don't mind.)

@JulianThijssen JulianThijssen force-pushed the feature/rename branch 5 times, most recently from 8be120b to 98e5546 Compare October 11, 2023 13:22
@JulianThijssen JulianThijssen merged commit c439f6d into master Oct 11, 2023
3 of 6 checks passed
@JulianThijssen JulianThijssen deleted the feature/rename branch October 11, 2023 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
amendment A change to the code to improve the quality core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants