EDM generation broken after install about podio

[drbenmorgan]

Hi @gaede, apologies for that, I'll get a fix Pull Request prepared.

[drbenmorgan]

Having had a quick look at this, there are a several ways to resolve this so thought better to discuss here before fixing in a Pull Request:

1. Restore original behaviour, so the install tree would look like:

   +- CMAKE_INSTALL_PREFIX/
      +- python/
      +- templates/
   

   The simplest, but will cause problems for packagers - e.g. say the install prefix is /usr/local, then /usr/local/templates is non-standard and could easily clash (I'm ignoring the python dir for now!).

2. Move templates as a subdirectory of python, so the relative path becomes "./templates". This is probably the best solution if the template files aren't for general access/use, i.e. only podio's python code will read them.

3. Update the relative path on install using known install locations to construct a relative path.

I'll implement whatever solution is preferred for podio. My personal recommendation would be the second one if the template files are only intended for internal use by podio's class generation.

[gaede]

Hi @drbenmorgan, I'd also vote for option 2). Can't think of any other use cases for the templates than from our python generators.

[gaede]

PS: shouldn't the INSTALL_DIR/python have a podio subdirectory in case someone wants to install to /usr/local/ ?

[drbenmorgan]

Thanks @gaede, I'll go ahead and implement 2).

You're right on the INSTALL_DIR/python issue, but python/podio also seems odd. Looking at the files in python/, there are some programs and modules

• programs: podio_class_generator.py, podio_create_package.py
• modules: podio_config_reader.py, podio_templates.py, EventStore.py

The former should probably go under a bin/ subdirectory, but am not sure about the latter. I'd guess lib/pythonX.Y/site-packages/podio. Maybe just install them all to bin for now?

[drbenmorgan]

Ah scratch that install to bin, since that would stick templates under that which wouldn't be good.

[gaede]

How about having sth like:

/usr/local/include
/usr/local/lib
/usr/local/podio
/usr/local/podio/python
/usr/local/podio/templates
/usr/local/podio/...

Then we could easily add whatever is needed to /usr/local/podio/ ?

[drbenmorgan]

I've submitted a Pull Request to fix the main issue using the 2nd option. Long term, it's probably best to think about a structure like:

+- bin/
    +- podio_class_generator
+- include/
    +- podio/
+- lib/
    +- libpodio.so
    +- ...dict...
    +- cmake/
    +- pythonX.Y/
       +- site-packages/
          +- podio/
             +- EventStore.py ?
             +- podio_config_reader.py ?
                 ...

That would require a little more work though, so thought best to submit the PR to get the fix in directly for now!

[peremato]

+1 On 27 Sep 2019, at 15:16, Ben Morgan <[email protected]<mailto:[email protected]>> wrote: I've submitted a Pull Request to fix the main issue using the 2nd option. Long term, it's probably best to think about a structure like: +- bin/ +- podio_class_generator +- include/ +- podio/ +- lib/ +- libpodio.so +- ...dict... +- cmake/ +- pythonX.Y/ +- site-packages/ +- podio/ +- EventStore.py ? +- podio_config_reader.py ? ... That would require a little more work though, so thought best to submit the PR to get the fix in directly for now! — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#58?email_source=notifications&email_token=ABVBMMSVS7DPVWVV5J7NC4DQLYBTFA5CNFSM4I3D6IEKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7Y3M4Q#issuecomment-535934578>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABVBMMSDJIZ6MGPLQC5JYK3QLYBTFANCNFSM4I3D6IEA>.