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

Make main.py executable and create a symlink during installation #21

Merged
merged 5 commits into from
Sep 29, 2024

Conversation

tmadlener
Copy link
Contributor

@tmadlener tmadlener commented Sep 27, 2024

BEGINRELEASENOTES

  • Mark main.py as executable and create a symlink from <prefix>/bin/k4GeneratorsConfig to <prefix>/python/main.py during installation. This makes it possible to setup an environment where k4GeneratorsConfig can be used as a simple executable
  • Remove some PYTHONPATH modifications from within the interpreter and use absolute imports. This makes it possible to simply add <prefix>/pyhton to the PYTHONPATH and everything will work.
    • Now we use relative imports internally mostly and make sure to expose Generators.Generators via settings in __init__.py

ENDRELEASENOTES

One thing I discovered that simply calling <prefix>/bin/k4GeneratorsConfig without argument, leads to:

$ ./install/bin/k4GeneratorsConfig 
Generating and writing configuration files
Traceback (most recent call last):
  File "/home/tmadlener/work/key4hep/k4GeneratorsConfig/./install/bin/k4GeneratorsConfig", line 158, in <module>
    main()
  File "/home/tmadlener/work/key4hep/k4GeneratorsConfig/./install/bin/k4GeneratorsConfig", line 102, in main
    executeFiles(files, 0, rndmSeed, events)
  File "/home/tmadlener/work/key4hep/k4GeneratorsConfig/./install/bin/k4GeneratorsConfig", line 154, in executeFiles
    return rndmIncrement
           ^^^^^^^^^^^^^
UnboundLocalError: cannot access local variable 'rndmIncrement' where it is not associated with a value

Ideally this should simply spit out a usage message, rather than error. Things work with -h or --help. Maybe there is an easy way to fix this, by e.g. making some of the arguments required, in which case argparse would do the right thing.

tmadlener and others added 2 commits September 27, 2024 11:47
Make sure that main.py doesn't have to be changed by exposing the
Generators.Generators class via __init__.py
Add protection against no -f argument
@dirkzerwas
Copy link
Collaborator

Protection added against -f FILENAME absent on command line

@tmadlener
Copy link
Contributor Author

Is not passing an input yaml file a "user error"? I.e. is the expectation that there should always be at least one yaml file? Because then we can just change nargs="*", to nargs="+".


install(CODE "execute_process(COMMAND bash -c \"set -e
link_target=${CMAKE_INSTALL_PREFIX}/python/main.py
link_name=${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_BINDIR}/k4GeneratorsConfig
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
link_name=${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_BINDIR}/k4GeneratorsConfig
link_name=${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_BINDIR}/k4generatorsConfig

I just realized that the alias in setup.sh is capitalized this way. I have no preference either way, it just has to be consistent in the end. @dirkzerwas any preferences from your side?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Your choice makes more sense, let's leave it as "k4GeneratorsConfig" , I will update the setups and tests to be consistent.

@dirkzerwas dirkzerwas merged commit d2e8026 into main Sep 29, 2024
2 of 4 checks passed
@dirkzerwas dirkzerwas deleted the symlink-main branch September 29, 2024 08:59
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.

2 participants