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

Adding new library legacylib #230

Merged
merged 8 commits into from
Jun 2, 2022
Merged

Adding new library legacylib #230

merged 8 commits into from
Jun 2, 2022

Conversation

juanangp
Copy link
Member

@juanangp juanangp commented May 26, 2022

juanangp Ok: 31

New legacy library has been added to the repository, check https://github.com/rest-for-physics/legacylib for more details.

The new legacy library is meant to store deprecated libraries in order to keep backward compatibility of the data with the following conditions:

  • A legacy class can be instanciated, in this case a warning in displayed.
  • A legacy class doesn't implement any function e.g. "ProcessEvent", an error is displayed and the program exit in case.
  • A legacy class is capable to print any metadata.
  • All legacy classes inherits from TRestLegacyProcess, which ensure the proper handling of legacy classes.

For the time being only TRestRawZeroSuppresionProcess.h is implemented as legacy class, however more classes can be added following this template. A base class and a template for TRestLegacyMetadata can be provided in case of need.

Also, when REST has not been compiled with all libraries, when trying to read a file that contains classes from the missing libraries, it would produce a seg.fault. This is fixed now by @jgalan

  • Added method REST_StringHelper::FirstToUpper.

@juanangp juanangp requested review from jgalan, nkx111 and lobis May 26, 2022 12:22
.gitmodules Outdated Show resolved Hide resolved
@jgalan jgalan self-requested a review May 26, 2022 14:22
@juanangp
Copy link
Member Author

juanangp commented May 26, 2022

Just realize that not all the libs are build by default, from https://github.com/rest-for-physics/framework/blob/master/.gitlab-ci.yml I can see:

 cmake ${CI_PROJECT_DIR} -DCMAKE_INSTALL_PREFIX=${CI_PROJECT_DIR}/install
      -DREST_GARFIELD=ON -DREST_G4=ON -DRESTLIB_DETECTOR=ON -DRESTLIB_RAW=ON -DRESTLIB_TRACK=ON -DRESTLIB_AXION=ON -DREST_WELCOME=OFF

Which leds to:
image

As you can see, some libraries such as geant4 or connectors are not enabled at this stage of the build. However, I think they are enabled lated on due to some dependencies.
I would suggest to enable all the libraries by default in the pipeline e.g.:

 cmake ${CI_PROJECT_DIR} -DCMAKE_INSTALL_PREFIX=${CI_PROJECT_DIR}/install -DREST_ALL_LIBS=ON -DREST_GARFIELD=ON -DREST_G4=ON -DREST_WELCOME=OFF

Same applies to the test, @lobis @jgalan let me know what do you think, I can do these changes and test in this PR.

@lobis
Copy link
Member

lobis commented May 26, 2022

Just realize that not all the libs are build by default, from https://github.com/rest-for-physics/framework/blob/master/.gitlab-ci.yml I can see:

 cmake ${CI_PROJECT_DIR} -DCMAKE_INSTALL_PREFIX=${CI_PROJECT_DIR}/install
      -DREST_GARFIELD=ON -DREST_G4=ON -DRESTLIB_DETECTOR=ON -DRESTLIB_RAW=ON -DRESTLIB_TRACK=ON -DRESTLIB_AXION=ON -DREST_WELCOME=OFF

Which leds to: image

As you can see, some libraries such as geant4 or connectors are not enabled at this stage of the build. However, I think they are enabled lated on due to some dependencies. I would suggest to enable all the libraries by default in the pipeline e.g.:

 cmake ${CI_PROJECT_DIR} -DCMAKE_INSTALL_PREFIX=${CI_PROJECT_DIR}/install -DREST_ALL_LIBS=ON -DREST_GARFIELD=ON -DREST_G4=ON -DREST_WELCOME=OFF

Same applies to the test, @lobis @jgalan let me know what do you think, I can do these changes and test in this PR.

I think its fine to add all libraries. Personally I like to have them all explicitly stated instead of having a single parameter for all of them, just in case we need to turn a single one off for some issue etc.

@juanangp
Copy link
Member Author

I think its fine to add all libraries. Personally I like to have them all explicitly stated instead of having a single parameter for all of them, just in case we need to turn a single one off for some issue etc.

Well, in this case you can always do:

 cmake ${CI_PROJECT_DIR} -DCMAKE_INSTALL_PREFIX=${CI_PROJECT_DIR}/install -DREST_ALL_LIBS=ON -DREST_GARFIELD=ON -DREST_G4=ON -DREST_WELCOME=OFF -DRESTLIB_LEGACY=OFF

At least for me is more clear this way.

@jgalan
Copy link
Member

jgalan commented May 26, 2022

Just off-topic, but when reformating XML files, it adds 2-spaces, should not be 4-spaces as C-code?

Also it puts everything really compactified, I really like to allow at least 1-line space between definitions

  </TRestSpiderMask>
  <TRestSpiderMask name="spider2" verboseLevel="info">
    <parameter name="maskRadius" value="20cm"/>
    <parameter name="offset" value="(0,0)cm"/>
    <parameter name="rotationAngle" value="0"/>
    <parameter name="armsWidth" value="5deg"/>
    <parameter name="armsSeparationAngle" value="30degrees"/>
    <parameter name="initialRadius" value="3cm"/>
  </TRestSpiderMask>
  <TRestSpiderMask name="spider3" verboseLevel="info">
    <parameter name="maskRadius" value="20cm"/>
    <parameter name="offset" value="(0,0)cm"/>
    <parameter name="rotationAngle" value="30deg"/>
    <parameter name="armsWidth" value="5deg"/>
    <parameter name="armsSeparationAngle" value="60degrees"/>
    <parameter name="initialRadius" value="6cm"/>
  </TRestSpiderMask>
</masks>

@lobis
Copy link
Member

lobis commented May 26, 2022

Just off-topic, but when reformating XML files, it adds 2-spaces, should not be 4-spaces as C-code?

Also it puts everything really compactified, I really like to allow at least 1-line space between definitions

  </TRestSpiderMask>
  <TRestSpiderMask name="spider2" verboseLevel="info">
    <parameter name="maskRadius" value="20cm"/>
    <parameter name="offset" value="(0,0)cm"/>
    <parameter name="rotationAngle" value="0"/>
    <parameter name="armsWidth" value="5deg"/>
    <parameter name="armsSeparationAngle" value="30degrees"/>
    <parameter name="initialRadius" value="3cm"/>
  </TRestSpiderMask>
  <TRestSpiderMask name="spider3" verboseLevel="info">
    <parameter name="maskRadius" value="20cm"/>
    <parameter name="offset" value="(0,0)cm"/>
    <parameter name="rotationAngle" value="30deg"/>
    <parameter name="armsWidth" value="5deg"/>
    <parameter name="armsSeparationAngle" value="60degrees"/>
    <parameter name="initialRadius" value="6cm"/>
  </TRestSpiderMask>
</masks>

Yes I agree, I also prefer 4 spaces. I made a commit to this branch that should modify this (6caf746), can you please check it works?

Regarding the separation, I am not sure how to modify this. It seems that xmllint tool is really old and does not have many configuration settings, perhaps there is a better tool for this?

@juanangp
Copy link
Member Author

Regarding the separation, I am not sure how to modify this. It seems that xmllint tool is really old and does not have many configuration settings, perhaps there is a better tool for this?

Just checked, it is working as expected.

@jgalan jgalan added this to the Release v2.3.13 milestone May 27, 2022
@nkx111
Copy link
Member

nkx111 commented May 29, 2022

Talking about legacy, I guess the old V2.2 processes and events can be also added? If it is possible, then we can recover backward compatibility to v2.2.

@juanangp
Copy link
Member Author

Talking about legacy, I guess the old V2.2 processes and events can be also added? If it is possible, then we can recover backward compatibility to v2.2.

Yes, this should be possible.

@juanangp juanangp merged commit b5f2709 into master Jun 2, 2022
@juanangp juanangp deleted the legacylib branch June 2, 2022 14:06
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