Skip to content

Conversation

@iarsene
Copy link
Collaborator

@iarsene iarsene commented Jan 8, 2021

  1. PWGDQ utility classes moved from AnalysisCore to Analysis/Tasks/PWGDQ.
  2. Removed the reducedEventAnalysis.cxx obsolete task from Analysis/Tasks

@iarsene iarsene requested a review from jgrosseo as a code owner January 8, 2021 15:31
@jgrosseo
Copy link
Collaborator

jgrosseo commented Jan 8, 2021

The include files will not work like this. The library name has to match the top level directory name. We probably need to discuss how to restructure if we want to do this...

@iarsene
Copy link
Collaborator Author

iarsene commented Jan 8, 2021

So you mean to change PWGDQCore with PWGDQ ?

Copy link
Collaborator

@jgrosseo jgrosseo left a comment

Choose a reason for hiding this comment

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

To my understanding but @aphecetche may comment as well, we would need to put the includes into include/PWGDQCore similar e.g. Analysis/Core/include/AnalysisCore for the includes of the AnalysisCore library.
In addition, for the logic, it would be properly better to have the code itself in directory corresponding to that library, instead of having it mixed into the Analysis/Tasks/PWGDQ directory.
I guess we need to discuss how we want to the structure overall...

@jgrosseo
Copy link
Collaborator

jgrosseo commented Jan 9, 2021

This is not just an aesthetic question: in your case the includes like VarManager.h are currently not installed with make install because they are not found in the right place.

@iarsene
Copy link
Collaborator Author

iarsene commented Jan 10, 2021

Hi @jgrosseo,
Ok, maybe we can briefly discuss it tomorrow at the meeting.
Cheers, Ionut

@iarsene
Copy link
Collaborator Author

iarsene commented Jan 12, 2021

Hi, so after our discussion, i created the Analysis/PWGDQ. The headers are now in include/PWGDQCore and sources in src/

@jgrosseo jgrosseo merged commit 27a8e79 into AliceO2Group:dev Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants