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

Fix 440 EICRecon compilation errors if used as cmake subproject #441

Merged
merged 2 commits into from
Jan 28, 2023

Conversation

DraTeots
Copy link
Contributor

In Cmake not there is EICRECON_SOURCE_DIR points to repository root which is used in jana_plugin.cmake and other places that need this particular place to be referenced

Briefly, what does this PR introduce?

What kind of change does this PR introduce?

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

no

Does this PR change default behavior?

no

EICRECON_SOURCE_DIR  points to repository root
@DraTeots DraTeots linked an issue Jan 16, 2023 that may be closed by this pull request
@DraTeots DraTeots changed the title Added EICRECON_SOURCE_DIR to cmake Fix 440 EICRecon compilation errors if used as cmake subproject Jan 16, 2023
@DraTeots DraTeots marked this pull request as ready for review January 16, 2023 18:56
Copy link
Contributor

@wdconinc wdconinc left a comment

Choose a reason for hiding this comment

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

I'm surprised there is a need for a new EICRECON_SOURCE_DIR. CMake provides so many *_DIR variables already that I doubt this is necessary...

@DraTeots
Copy link
Contributor Author

DraTeots commented Jan 17, 2023

I'm surprised there is a need for a new EICRECON_SOURCE_DIR. CMake provides so many *_DIR variables already that I doubt this is necessary...

Usually they point to root or current project. Not specific if EICRECON is in the middle of the chain (it is not root anymore). There is a named _SOURCE_DIR but I'll explain further why introducing a variable is better.

https://cmake.org/cmake/help/latest/variable/PROJECT-NAME_SOURCE_DIR.html?highlight=project%20dir

Indeed it is better to be renamed to EICRECON_ROOT_DIR or EICRECON_HOME_DIR (As things named in jana). From the user point or usage points (like specific plugin CMake) it is better to rely on a special variable rather than on some project name. First it shows the intent better (and the intent to get ROOT of EICRecon dir rather than some specific project EICRecon). And the same goes to reliability - it is better to rely on a variable designed for it rather than on a fact where the project is located. Finally there are 2 eicrecon projects. One is the root project and the secon is executable eicrecon. So I think introducing EICRECON_ROOT_DIR is good from reliability, readability and unambiguousness points.

@wdconinc
Copy link
Contributor

Which project (besides your clion settings) is this expected to affect?

@DraTeots
Copy link
Contributor Author

Any project or setup that does add_subdirectory(EICRecon) will fail.

Copy link
Contributor

@wdconinc wdconinc left a comment

Choose a reason for hiding this comment

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

Since EICrecon will be reworked, let's just approve this and free up the queue for other issues and pull requests.

@wdconinc wdconinc enabled auto-merge January 28, 2023 03:17
@wdconinc wdconinc merged commit 9e4f85f into main Jan 28, 2023
@wdconinc wdconinc deleted the 440-compilation-error-if-eicrecon-cmake-subproject branch January 28, 2023 03:38
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.

Compilation error if EICrecon is used as cmake subproject
3 participants