chamberInfo.py should be replaced when the rpm is installed

cms-gem-daq-project / gem-plotting-tools

Repository for GEM commissioning plotting tools

GNU General Public License v3.0

1 stars 26 forks source link

chamberInfo.py should be replaced when the rpm is installed #267

Closed AndrewLevin closed 4 years ago

AndrewLevin commented 4 years ago

Description

This pull request simply removes the line that prevents chamberInfo.py from being overwritten.

Types of changes

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

Motivation and Context

If chamberInfo.py is not replaced, new features in chamberInfo.py are not installed.

How Has This Been Tested?

Yes, I have installed the package in my virtual environment

Screenshots (if appropriate):

Checklist:

[x] 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.

AndrewLevin commented 4 years ago

Yes, I guess that commented line is for installation in a virtual environment, although in my virtual environment I don't actually have an environment variable $PYTHON2_SITELIB.

lpetre-ulb commented 4 years ago

Yes, I guess that commented line is for installation in a virtual environment, although in my virtual environment I don't actually have an environment variable $PYTHON2_SITELIB.

Indeed, it is designed to automatically work with different Python site-packages locations. The main target is the support of different Python versions. The variable will correctly refer to your virtual environment, even if an RPM should never refer to a specific virtual environment...

The syntax %{python2_sitelib} is a RPM macro (and not an environment variable) which is defined as: python2 -c "from distutils.sysconfig import get_python_lib; print(get_python_lib())".

jsturdy commented 4 years ago

This behaviour is actually by design, (because this file holds configuration information) it should not overwrite a potentially system specific file. When installing, if rpm notices that there are differences, it will create rpmnew and rpmsave files, but it will not replace the current file cf, and when this happens, the command will tell you that it happened, so it's up to the sysadmin to notice that there are changes and resolve them, if desired.

The %config(noreplace) must stay, and using the proper rpm macros would be good, it just needs to be validated against a future (cc8) world where we'll provide a python3 package

AndrewLevin commented 4 years ago

At least in our QC7 and QC8 setups, the system specific information is stored in a separate file called system_specific_constants.py, no in chamberInfo.py.

jsturdy commented 4 years ago

OK, if we've completely removed any "editable" features from this file, then I jumped the gun on closing this.

lpetre-ulb commented 4 years ago

Yes, all "editable" dictionaries are moved to the system_specific_constant module which must be present in the PYTHONPATH (or be importable somehow). chamberInfo.py is kept only for backwards compatibility throughout the code base and import the dictionaries from system_specific_constant for any setup-specific configuration.

AndrewLevin commented 4 years ago

It is possible to edit the dictionaries directly in the chamberInfo.py which will be used if the system_specific_constants.py is not found, but I think the proper way is to separate the system specific information like we are doing in all of the 904 setups.