-
Notifications
You must be signed in to change notification settings - Fork 60
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
EDM generation broken after install #58
Comments
Hi @gaede, apologies for that, I'll get a fix Pull Request prepared. |
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:
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. |
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. |
PS: shouldn't the |
Thanks @gaede, I'll go ahead and implement 2). You're right on the
The former should probably go under a |
Ah scratch that install to |
How about having sth like:
Then we could easily add whatever is needed to |
The changes introduced by AIDASoft#54 caused installs to fail due to the change in location of the templates directory relative to python/. Following discussion in AIDASoft#58, implement simplest possible change to fix by moving templates inside the python directory. Retain this layout at install time, adjusting the `template_dir` variable in podio_class_generator.py accordingly. Fixes: AIDASoft#58
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:
That would require a little more work though, so thought best to submit the PR to get the fix in directly for now! |
+1
On 27 Sep 2019, at 15:16, Ben Morgan <notifications@github.com<mailto:notifications@github.com>> 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>.
|
The changes introduced by #54 caused installs to fail due to the change in location of the templates directory relative to python/. Following discussion in #58, implement simplest possible change to fix by moving templates inside the python directory. Retain this layout at install time, adjusting the `template_dir` variable in podio_class_generator.py accordingly. Fixes: #58
As a result of #54 the EDM egenration does not work anymore for external clients (EDM4Hep).
The reason is the hardcoded template_dir path "../templates" that is no longer available in the install. @drbenmorgan can you have a look at this ?
Current workaround:
The text was updated successfully, but these errors were encountered: