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.

Sign up for GitHub

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

Jump to bottom

Console scripts #4

Closed
wants to merge 3 commits into from
Closed

Console scripts #4

wants to merge 3 commits into from

Conversation

cholmcc
Copy link
Contributor

@cholmcc cholmcc commented Feb 11, 2020

In this PR we create the using facing scripts (such as alien_cp) through the setuptools console-script mechanism. That is, the shell script present in bin/ of the repository are removed in favour of defining the entry-points in setup.py.

So what happens: When installing the package (via pip, directly via setup.py - f.ex. in develop mode), setuptools writes small wrappers to the target installation bin directory (e.g., /usr/local/bin, ~/.local/bin) which imports the appropriate module or package and executes the named function from that module.

As an example

entry_points = {
    'console_scripts': [
        'alien_cp = xjalienfs.alien:cmd_cp',
     }
}

will generate the console-script alien_cp which essentially does

import xjalienfs.alien 
xjalienfs.alien.cmp_cp()

To this end, we add a few (small) functions to alien.py - essentially doing, for example

def do_cp():
    sys.argv = sys.argv[:1] + ['cp'] + sys.argv[1:]
    main()

The benefits of using setuptools entry points like this is that it can correctly take care of platform idiosyncrasies, set-up proper load paths, and so on.

Remember, one can always install a package in developer mode (i.e., no scripts are actually copied to destination - instead a "pointer" is created at the destination that points back to the working directory. In this way, any changes made to the source is immediately reflected in the the "installation") by

python setup.py develop --user

The only script not rewritten like this is alice_pfn which remains a shell script that calls the entry point alien_which.

@cholmcc
Fix security level for 80bit server
12dd8c8
@cholmcc
Fixes for dependencies in setup.py
afd2c2b 
- `gnureadline` replaced by `readline`.  `gnureadline` is deprecated,
  and one should really only depend on `readline`.  The code is not
  changed so that if `gnureadline` is installed that will be picked
  up.  What's more, `gnureadline` is _explicitly_ wrong on Linux
  and will not work on Windoze.
- The Python interface to XRootD identifies it self as `xrootd`
  in the `egg` setup (not `pyxrootd`).  Fixed for `xrootd`.
@cholmcc
Replace shell scripts with setup-tools console-scripts
a8c2a59 
Most (well, all but `alien_pfn`) shell scripts in bin are
replaced by `setuptool` console scripts (i.e., scripts
automatically generated on install).  The scripts call
functions in the `alien.py` module - e.g.,

- `alien_cp` calls the function `xjalienfs.alien.do_cp` roughly
   coded as

      def do_cp():
          sys.argv = sys.argv[:1] + ['cp'] + sys.argv[1:]
          main()

This allows setup-tools to set-up proper load paths, interpreters,
and so on.  It also allows pip to correctly remove files on
uninstall, and so on.
@costing
Copy link

costing commented Feb 14, 2020

Hi @cholmcc,

I'm not sure I understand the purpose of this PR. Our target installation is CVMFS where dependencies are handled by the module files so I don't see the benefit of having to run a script to generate one line scripts, as there is nothing else for them to do anyway.

Cheers,

.costin

@adriansev adriansev closed this Feb 14, 2020
@cholmcc
Copy link
Contributor Author

cholmcc commented Feb 14, 2020

@costing, @adriansev

The purpose of the PR is to make the Python package follow the general guidelines offered for best practises of Python packages. This means that it is strongly encourage to use setuptools entry-points for scripts. This has several benefits

  • Platform independence. setuptools knows how to write appropriate scripts on all platforms.
  • Ease of installation. setuptools will install all script at appropriate places as dictated by the target
    machine, including appropriate for loading.
  • Code maintenance. The script code is moved into Python (as opposed to shell scripts or the like)
  • ... and many more (see the various web-pages and the [documentation[(https://setuptools.readthedocs.io/en/latest/setuptools.html#automatic-script-creation))

Concretely, for installations via alidst this would mean that the installation part of the xjalienfs.sh install script is changed from

env PYTHONUSERBASE="$INSTALLROOT" ALIBUILD=1 pip3 install --user file://${SOURCEDIR}
XJALIENFS_SITEPACKAGES=$(find ${INSTALLROOT} -name site-packages)

ALIEN_PY=$(find ${INSTALLROOT} -name alien.py)

cp -r $SOURCEDIR/bin ${INSTALLROOT}/bin
cp ${ALIEN_PY} ${INSTALLROOT}/bin/alien.py
chmod +x ${INSTALLROOT}/bin/*

to just

env PYTHONUSERBASE="$INSTALLROOT" ALIBUILD=1 pip3 install --user file://${SOURCEDIR}
XJALIENFS_SITEPACKAGES=$(env PYTHONUSERBASE="$INSTALLROOT" python3 -m site --user-site)

Note, I use Python to find the installation directory to set XJALIENFS_SITEPACKAGES.

Also note, when you run pip install file://... you are effectively doing python setup.py install so you are already running setup.py. In fact, it is the only way to make sure the package ends up the right place (see here). Try running

pip -v -v -v install file://...

to see what is going on.

Thus, adopting this PR would vastly simplify the installation of the package.

As I wrote in another PRs comments the idea is that the scripts are written at installation. To see what goes on, you can install the package with the PR package as

python setup.py install --user

This will generate the scripts

~/.local/bin/alien_cmd
~/.local/bin/alien_cp
~/.local/bin/alien_find
~/.local/bin/alien_guid2lfn
~/.local/bin/alien_lfn2guid
~/.local/bin/alien_ls
~/.local/bin/alien_mirror
~/.local/bin/alien_mkdir
~/.local/bin/alien_mv
~/.local/bin/alien_pfn
~/.local/bin/alien_ps
~/.local/bin/alien_rm
~/.local/bin/alien_rmdir
~/.local/bin/aliensh
~/.local/bin/alien_stat
~/.local/bin/alien_submit
~/.local/bin/alien-token-info
~/.local/bin/alien-token-init
~/.local/bin/alien_whereis

For development, one can do

python setup.py develop --user

This will create an "installation" that points to the current development directory, and changes will be immediately be reflected in the "installation".

Note, if one currently does

python setup.py install

or

pip install xjalienfs

(as expected by users) then one will end up with an incomplete installation. This is entirely unexpected by most users.

Finally, note that CVMFS is not the only "customer" of the package. Certainly others will also want to install the package - and some will also by-pass alidist (for good reason - alidist is a beast). If they get the package as is now and does

python setup.py install

then they will end up with a broken package.

The PR fixes all these issues with no cost (development can be done as before) while making the code much more maintainable.

Please reconsider this PR. If you want, I can make a new PR on top of current master if needed.

Christian

@costing
Copy link

costing commented Feb 14, 2020

Hi @cholmcc, @adriansev,

I can't say that moving one liners in Python is appealing to me. If anything I suggest something else, and I think Adrian has already something in this direction: make alien_* symlinks to alien.py and have the entry point look up the command from the name of $0. That to me looks like the best of both worlds: no bunch of scripts, no need for setting up anything.

As far as I understand the only missing point is how to add project's bin/ to $PATH so standalone installations can work out of the box. Here I let you two find the best solution.

Is this approach addressing the use cases you are considering?

Cheers,

.costin

@cholmcc
Copy link
Contributor Author

cholmcc commented Feb 14, 2020 via email

@costing
Copy link

costing commented Feb 15, 2020

Thanks a lot @cholmcc for the thorough explanation.

I find the platform independence of this solution a very strong point. @adriansev, could you please reconsider the PR?

Cheers,

.costin

@adriansev
Copy link
Owner

@cholmcc could you please resubmit the console-scripts part only, rebased on current master?
also, could you check if the PR work on EL7 systems? that security SECLEVEL patch did not work on EL7 (the only supported platform for both ALICE software and for all GRID services and installation) and i had to add a workaround. Thank you very much for your contributions!

@adriansev
Copy link
Owner

@cholmcc i realized that the same mechanics that recognize the format of sys.argv[0] i can use for all (almost all) entry points, so i setup the script names with the same entry point. can you please take a loot at master so see if there are any other problems?
Thank you for all your ideas and feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adriansev adriansev Awaiting requested review from adriansev

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@cholmcc @costing @adriansev