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

Added preprocessor definition via cmake STRING #185

Merged
merged 3 commits into from
Apr 22, 2022

Conversation

lobis
Copy link
Member

@lobis lobis commented Apr 21, 2022

lobis Ok: 7

Related to https://rest-forum.unizar.es/t/detectorlib-implementation-problem-inside-the-rawlib-process/535/5

Adding definitions via CMake which appear only when corresponding libraries are selected to compile. This is useful when we want to have code in one library that uses another library that may or may not be available.

I tried to look for where this was being defined previously in the code but didn't find it (for any library) I guess it was removed at some point and never cause problems.

@jgalan
Copy link
Member

jgalan commented Apr 22, 2022

Ok, it seems to be working nice.

The only thing is the output is a bit distorted, how the DEBUG statement works? Perhaps its behaviour is related to the cmake version?

Empty submodule dir: packages/restSQL. Option: REST_SQL=OFF
-- making build files in /home/jgalan/rest-framework/source/libraries/axion, dependency: 
-- 20 source files in total, 20 classes to generete, 0 additional source files
DEBUGAdding compile definition: REST_AxionLib
-- making build files in /home/jgalan/rest-framework/source/libraries/detector, dependency: 
-- 37 source files in total, 37 classes to generete, 0 additional source files
DEBUGAdding compile definition: REST_DetectorLib

@jgalan jgalan requested a review from Andrii01L April 22, 2022 08:59
jgalan
jgalan previously approved these changes Apr 22, 2022
@lobis
Copy link
Member Author

lobis commented Apr 22, 2022

Ok, it seems to be working nice.

The only thing is the output is a bit distorted, how the DEBUG statement works? Perhaps its behaviour is related to the cmake version?

Empty submodule dir: packages/restSQL. Option: REST_SQL=OFF
-- making build files in /home/jgalan/rest-framework/source/libraries/axion, dependency: 
-- 20 source files in total, 20 classes to generete, 0 additional source files
DEBUGAdding compile definition: REST_AxionLib
-- making build files in /home/jgalan/rest-framework/source/libraries/detector, dependency: 
-- 37 source files in total, 37 classes to generete, 0 additional source files
DEBUGAdding compile definition: REST_DetectorLib

https://cmake.org/cmake/help/latest/command/message.html

In my cmake version it works fine, I guess this did not exist in your cmake version and thats why it concatenates the strings. DEBUG messages are intended for developers and not users, in my opinion they are a middle ground between STATUS messages and comments, you can change the DEBUG to STATUS to display the mesage easier than to change it from a comment to a message.

There are many small problems similar to this that can happen due to allowing users to use very old cmake versions, we should change the minimum cmake version but its not possible yet because of #162, I couldn't fix it. I encourage anyone @rest-for-physics/core_dev to try to fix this issue as I think its a bit critical.

juanangp
juanangp previously approved these changes Apr 22, 2022
@jgalan
Copy link
Member

jgalan commented Apr 22, 2022

I woudnt mind that the DEBUG statement is in STATUS statement. Anyway a non-developer user will not look most of the information coming out from cmake.

@lobis lobis dismissed stale reviews from juanangp and jgalan via f7ca4a7 April 22, 2022 11:03
@lobis
Copy link
Member Author

lobis commented Apr 22, 2022

I woudnt mind that the DEBUG statement is in STATUS statement. Anyway a non-developer user will not look most of the information coming out from cmake.

I made a commit for this and now the reviews have to be made again, I think this is new behavior right? @jgalan @juanangp

@jgalan
Copy link
Member

jgalan commented Apr 22, 2022

Yes, I was playing with the settings. If someone pushes new commits, then any approval will be removed, which I believe it makes sense, but if you want, we get back to previous setting? @juanangp @lobis ?

@lobis
Copy link
Member Author

lobis commented Apr 22, 2022

Yes, I was playing with the settings. If someone pushes new commits, then any approval will be removed, which I believe it makes sense, but if you want, we get back to previous setting? @juanangp @lobis ?

I think it makes sense, I agree 👍

@lobis lobis deleted the lobis-regex-lib-compile-def branch April 22, 2022 19:39
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.

None yet

4 participants