Install the man pages #139

lmoureaux · 2018-07-19T01:24:16Z

Description

This PR modifies the Makefile and setup script to install the man pages. They are installed at $BASE/share/man, where $BASE is as follows:

$VIRTUAL_ENV when installed within a virtualenv
/usr when installed system-wide

Man pages in the virtualenv can be accessed by setting the correct $MANPATH.

[outdated]
A side-effect of this change is that I figured out how to modify the default setuptools install location. All scripts and macros are now installed in $BASE/bin. This means that virtualenv can take care of updating the $PATH (system-wide installs still need a setup script).
[/outdated]

Another side-effect is that I corrected the license information in the package metadata. It was saying "MIT" while the LICENSE file always contained the text of GPLv3 (there is no way to change back to MIT without contacting every single contributor).

The setup script and developer documentation will need to be updated to use the $MANPATH.

Downsides

The man pages produced when the package is not installed may not be completely accurate. This is a problem for make rpm since it doesn't work inside a virtualenv.
[outdated]Macros and scripts are present twice in the package: under bin/ and under gempython/.... I couldn't figure out why.[/outdated]
CI is broken 😢

As a result of these downsides and my inability to fix them shortly, this PR is provided more as a list of commits to be cherry-picked.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Motivation and Context

Include man pages in the package #138

[outdated] Comment in setup.py:

would prefer to use this for executables, if one can control the install location
to be user defined rather than /usr/bin

The preferred method (native support from setuptools) is now used. [/outdated]

How Has This Been Tested?

The pip and rpm packages were built, the former from a virtualenv and the latter on gem904daq01.
The pip package was installed; scripts and man pages are correctly installed and have the correct permissions.
It was checked that the rpm package contains all files.
The man pages built in each environment were (quickly) checked.

Since I don't have access to a rpm-based machine and I don't want to install one, I couldn't check that the rpm package installs cleanly.

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

jsturdy

There are far too many changes in this PR that should have been discussed in #138 before being implemented

A side-effect of this change is that I figured out how to modify the default setuptools install location. All scripts and macros are now installed in $BASE/bin. This means that virtualenv can take care of updating the $PATH (system-wide installs still need a setup script).

This was by design, avoiding putting things in macros into /opt/cmsgemos/bin

Another side-effect is that I corrected the license information in the package metadata. It was saying "MIT" while the LICENSE file always contained the text of GPLv3 (there is no way to change back to MIT without contacting every single contributor).

This is probably the discussion that I mention inline. I would have to go back in time to figure out why a given license was (probably arbitrarily) chosen for this project, and probably raise it as a discussion, (contacting contributors on this project is pretty trivial)

jsturdy · 2018-07-19T06:40:48Z

pkg/installrpm.sh

@@ -23,8 +23,8 @@ cat INSTALLED_FILES.backup|egrep -v 'gempython/__init__.py*' > INSTALLED_FILES
 # set permissions
 cat <<EOF >>INSTALLED_FILES
 %attr(0755,root,root) /opt/cmsgemos/bin/*.py
-%attr(0755,root,root) /usr/lib/python*/site-packages/gempython/scripts/*.py
-%attr(0755,root,root) /usr/lib/python*/site-packages/gempython/gemplotting/macros/*.py
+%attr(0755,root,root) /opt/cmsgemos/lib/python*/site-packages/gempython/scripts/*.py


This was also explicitly avoided in the initial setup, as in future versions this will be replaced with system site-packages location macros provided by rpm (currently only works for cc7)

system site-packages location macros provided by rpm

Could you provide more details about this? A link to the rpm doc about this would be great

jsturdy · 2018-07-19T06:40:56Z

pkg/setup.cfg

@@ -4,7 +4,7 @@ name = gempython_gemplotting
 author = GEM Online Systems Group
 author_email = [email protected]
 summary = __summary__
-license = MIT
+license = GNU General Public License v3 (GPLv3)


License changes must be discussed

This is not a license change, this is reflecting the fact that the current license is GPLv3. If you want to contact all copyright holders, fine; but until you get answers the license remains GPLv3.

jsturdy · 2018-07-19T06:41:08Z

pkg/setup.cfg

@@ -20,7 +20,8 @@ classifier =
    Topic :: Data Acquisition
    Topic :: Scientific
    Topic :: Utilities
-    License :: OSI Approved :: MIT
+    License :: OSI Approved :: GNU General Public License (GPL)


jsturdy · 2018-07-19T06:50:34Z

pkg/setup.py

+    # Man files
+    man_sections = {}
+    for file in listdir(mandir):
+        print('Found man page: %s' % file)


Also, prefer consistent string formatting (avoid % formatting in favour of .format() formatting) for all current and future contributions, CONTRIBUTING.md will be updated to reflect this and a few other changes

For my own ignorance, why is .format() viewed as superior?

I don't know about superior... it is the standard that was introduced to eventually replace % formatting, though I suspect that % formatting will stick around for quite some time...
Both have a few advantages, though I think .format() has more.
The primary thing here (in this comment) is maintaining consistency

lmoureaux · 2018-07-19T10:02:50Z

avoiding putting things in macros into /opt/cmsgemos/bin

May I ask what's the rationale for the difference between scripts and macros in this project?

lmoureaux · 2018-07-19T11:46:32Z

I would have to go back in time to figure out why a given license was (probably arbitrarily) chosen for this project

Commit 3d58f5d by... @jsturdy. The commit message ("Initial commit") isn't helpful in telling how the license was chosen.

FYI:

CMSSW uses Apache 2.0.
XDAQ is under a 3-clause BSD-style license

jsturdy · 2018-07-19T11:48:38Z

May I ask what's the rationale for the difference between scripts and macros in this project?

Mainly, it is to isolate the key executable parts (analyzing scans, which should go in /opt/cmsgemos/bin) from random plotting and style macros (can still be on the PATH for ease of use, provided PATH is modified to include the macros directory)

Further down the road, much of this will be replaced with console_scripts and entry_points, but that is a minor priority

It looks like it's required when installing them in the system location. See issue cms-gem-daq-project#138 and PR cms-gem-daq-project#139.

lmoureaux · 2018-07-19T13:43:30Z

Removed the controversial move to a more standard behavior using standard tools; the rpm package will now install man pages in /usr/share. Updated the first post accordingly.

See PR cms-gem-daq-project#139

The LICENSE files contains GPLv3, not MIT

See issue cms-gem-daq-project#138

It looks like it's required when installing them in the system location. See issue cms-gem-daq-project#138 and PR cms-gem-daq-project#139.

See PR cms-gem-daq-project#139

This reverts commit 1fdebc0. It didn't work with Python 2.6.

lmoureaux · 2018-10-16T13:44:13Z

Comments have been addressed and I'm waiting for another review.

jsturdy

RPM requirements:

rpm -q --requires -p rpm/gempython_gemplotting-1.0.0-0.0.6.30.dev5.087aeffgit.centos7.python2.7.noarch.rpm
/bin/env
/bin/sh
/usr/bin/env
cmsgemos_gempython >= 0.3.1
config(gempython_gemplotting) = 1.0.0-0.0.6.30.dev5.087aeffgit.centos7.python2.7
numpy >= 1.7
python >= 2.6
python(abi) = 2.7
root_numpy >= 4.7
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PartialHardlinkSets) <= 4.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
tabulate >= 0.4.4
rpmlib(PayloadIsXz) <= 5.2-1

looks like some mixed shebangs are used, these should be cleaned up in the future
also in the future, would like to be able to provide these to bdist_rpm based on the python version, but I believe this would be a significant redesign, moving the majority of the setup.cfg stuff into functions imported in setup.py

It looks like it's required when installing them in the system location. See issue #138 and PR #139.

See PR #139

bdorney added the Status: In Progress label Jul 19, 2018

bdorney assigned lmoureaux Jul 19, 2018

jsturdy suggested changes Jul 19, 2018

View reviewed changes

jsturdy added Status: Blocked Status: Revision Needed labels Jul 19, 2018

lmoureaux force-pushed the feature_installManPages branch from 20a53ab to cc83a72 Compare July 19, 2018 13:36

lmoureaux added a commit to lmoureaux/gem-plotting-tools that referenced this pull request Jul 19, 2018

Compress man pages

cc83a72

It looks like it's required when installing them in the system location. See issue cms-gem-daq-project#138 and PR cms-gem-daq-project#139.

lmoureaux added a commit to lmoureaux/gem-plotting-tools that referenced this pull request Jul 19, 2018

% -> format() in setup.py

0a276c3

See PR cms-gem-daq-project#139

jsturdy mentioned this pull request Jul 20, 2018

Relicensing as MIT (was GPLv3) #142

Open

6 tasks

lmoureaux added 4 commits August 13, 2018 18:41

Correct license information in package metadata

600b949

The LICENSE files contains GPLv3, not MIT

Add the man pages to the pip and rpm packages

26e9307

See issue cms-gem-daq-project#138

Compress man pages

3a27267

It looks like it's required when installing them in the system location. See issue cms-gem-daq-project#138 and PR cms-gem-daq-project#139.

% -> format() in setup.py

1fdebc0

See PR cms-gem-daq-project#139

lmoureaux force-pushed the feature_installManPages branch from 0a276c3 to 9ed4e3b Compare August 13, 2018 16:58

lmoureaux added a commit to lmoureaux/gem-plotting-tools that referenced this pull request Aug 13, 2018

Try to fix Travis build

9ed4e3b

See PR cms-gem-daq-project#139

lmoureaux force-pushed the feature_installManPages branch from 9ed4e3b to 2a56153 Compare August 13, 2018 17:09

lmoureaux added a commit to lmoureaux/gem-plotting-tools that referenced this pull request Aug 13, 2018

Try to fix Travis build

2a56153

See PR cms-gem-daq-project#139

lmoureaux force-pushed the feature_installManPages branch from 2a56153 to a0857c1 Compare August 13, 2018 17:31

lmoureaux added a commit to lmoureaux/gem-plotting-tools that referenced this pull request Aug 13, 2018

Try to fix Travis build

a0857c1

See PR cms-gem-daq-project#139

lmoureaux force-pushed the feature_installManPages branch from a0857c1 to cbd8694 Compare August 13, 2018 17:49

lmoureaux added a commit to lmoureaux/gem-plotting-tools that referenced this pull request Aug 13, 2018

Try to fix Travis build

cbd8694

See PR cms-gem-daq-project#139

lmoureaux force-pushed the feature_installManPages branch from cbd8694 to 6409f25 Compare August 13, 2018 18:13

lmoureaux added a commit to lmoureaux/gem-plotting-tools that referenced this pull request Aug 13, 2018

Try to fix Travis build

6409f25

See PR cms-gem-daq-project#139

lmoureaux force-pushed the feature_installManPages branch from 6409f25 to f50904f Compare August 13, 2018 18:23

lmoureaux added a commit to lmoureaux/gem-plotting-tools that referenced this pull request Aug 13, 2018

Try to fix Travis build

f50904f

See PR cms-gem-daq-project#139

Try to fix Travis build

23cb5e0

See PR cms-gem-daq-project#139

lmoureaux force-pushed the feature_installManPages branch from f50904f to 23cb5e0 Compare August 13, 2018 19:01

Revert "% -> format() in setup.py"

087aeff

This reverts commit 1fdebc0. It didn't work with Python 2.6.

jsturdy changed the title ~~WIP: Install the man pages~~ Install the man pages Oct 17, 2018

jsturdy approved these changes Oct 17, 2018

View reviewed changes

jsturdy merged commit 69a710d into cms-gem-daq-project:develop Oct 17, 2018

jsturdy pushed a commit that referenced this pull request Oct 17, 2018

Compress man pages

c0793fa

It looks like it's required when installing them in the system location. See issue #138 and PR #139.

jsturdy pushed a commit that referenced this pull request Oct 17, 2018

% -> format() in setup.py

c73acc0

See PR #139

jsturdy pushed a commit that referenced this pull request Oct 17, 2018

Try to fix Travis build

3b8fd33

See PR #139

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Install the man pages #139

Install the man pages #139

lmoureaux commented Jul 19, 2018 •

edited

Loading

jsturdy left a comment •

edited

Loading

jsturdy Jul 19, 2018

lmoureaux Jul 19, 2018 •

edited

Loading

jsturdy Jul 19, 2018

lmoureaux Jul 19, 2018

jsturdy Jul 19, 2018

jsturdy Jul 19, 2018

bdorney Jul 19, 2018

jsturdy Jul 19, 2018

lmoureaux commented Jul 19, 2018

lmoureaux commented Jul 19, 2018

jsturdy commented Jul 19, 2018

lmoureaux commented Jul 19, 2018

lmoureaux commented Oct 16, 2018

jsturdy left a comment •

edited

Loading

Install the man pages #139

Install the man pages #139

Conversation

lmoureaux commented Jul 19, 2018 • edited Loading

Description

Downsides

Types of changes

Motivation and Context

How Has This Been Tested?

Checklist:

jsturdy left a comment • edited Loading

Choose a reason for hiding this comment

jsturdy Jul 19, 2018

Choose a reason for hiding this comment

lmoureaux Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

jsturdy Jul 19, 2018

Choose a reason for hiding this comment

lmoureaux Jul 19, 2018

Choose a reason for hiding this comment

jsturdy Jul 19, 2018

Choose a reason for hiding this comment

jsturdy Jul 19, 2018

Choose a reason for hiding this comment

bdorney Jul 19, 2018

Choose a reason for hiding this comment

jsturdy Jul 19, 2018

Choose a reason for hiding this comment

lmoureaux commented Jul 19, 2018

lmoureaux commented Jul 19, 2018

jsturdy commented Jul 19, 2018

lmoureaux commented Jul 19, 2018

lmoureaux commented Oct 16, 2018

jsturdy left a comment • edited Loading

Choose a reason for hiding this comment

lmoureaux commented Jul 19, 2018 •

edited

Loading

jsturdy left a comment •

edited

Loading

lmoureaux Jul 19, 2018 •

edited

Loading

jsturdy left a comment •

edited

Loading