sclorg / rpm-list-builder

RPM List Builder helps you to build a list of defined RPM packages including Software Collection from the recipe file
GNU General Public License v2.0
4 stars 8 forks source link

Clarify that the current environment is passed down to custom hooks #81

Open ncoghlan opened 7 years ago

ncoghlan commented 7 years ago

(Calling this an "idea" rather than a request for enhancement, as I don't actually need it right now, it's just a possibility that occurred to me while working on #67).

The custom download and build hook support in rpmlb is excellent, and makes it possible to do some quite interesting things with custom hook files.

However, it's currently difficult to pass parameters to those hook files, such as paths to configuration files, fedpkg branches to use, etc.

The idea that occurred to me while working on adding the CUSTOM_DIR environment variable is that it may be useful to be able to pass -e ENV_VAR=value on the command line and have that setting be passed through to the execution environment for the hooks (similar to the way PKG and CUSTOM_DIR get set).

ncoghlan commented 7 years ago

As a more concrete example, in https://github.com/ncoghlan/pyscl-devel/blob/master/rpmlb/sclorg-distgit-download.yml I have a command that looks like this:

fedpkg --release el7 --module-name ${PKG} srpm

With arbitrary environment variable support, I could instead write that line like this:

fedpkg --release ${SRPM_DIST:=el7} --module-name ${PKG} srpm

By default, that would still use el7 as the value of SRPM_DIST, but it could also be overwritten from the command line:

rpmlb --custom-file hooks.yml --build custom -e SRPM_DIST=el6

As noted in the first post, I don't actually need this right now, but it's conceivable I may want it once I start building for el6 as well as el7.

junaruga commented 7 years ago

@ncoghlan

The idea that occurred to me while working on adding the CUSTOM_DIR environment variable is that it may be useful to be able to pass -e ENV_VAR=value on the command line and have that setting be passed through to the execution environment for the hooks (similar to the way PKG and CUSTOM_DIR get set).

I agree with you. Thank you for the concrete example. I could understand your situation.

Right now below command may work to use the environment variable in the custom file.

$ SRPM_DIST=el6 rpmlb --custom-file hooks.yml --build custom

But the -e|--env option looks useful.

How about a case to set 2 environment variables?

Like this? Like a docker run -e option. [1]

$ rpmlb --custom-file hooks.yml --build custom -e SRPM_DIST=el6 -e FOO=BAR

[1] https://stackoverflow.com/questions/30494050/how-do-i-pass-environment-variables-to-docker-containers/30494145

hroncok commented 7 years ago

What is the current behavior if you set the environment variable like @junaruga just showed? Is it propagated inside?

$ SRPM_DIST=el6 rpmlb --custom-file hooks.yml --build custom
ncoghlan commented 7 years ago

@hroncok You know, I didn't even check that - I just assumed it worked the same way as Python subprocesses and never even tried it.

However it turns out that rpmlb already copies the current environment before updating it with any extra variables like PKG or CUSTOM_DIR, so this is actually just a documentation update request rather than a feature request:

$ CUSTOM_VAR=HELLO rpmlb --custom-file custom-env-var-hooks.txt --download custom --build custom custom-env-var-recipe.txt custom-env-var
2017-07-25 22:14:08,097 INFO     rpmlb.app                  Starting rpmlb (1.0.0)
2017-07-25 22:14:08,100 INFO     rpmlb.work                 Working directory: /tmp/rpmlb-i7le1941
2017-07-25 22:14:08,101 INFO     rpmlb.app                  Downloading...
HELLO before download
HELLO during package1 download
HELLO during package2 download
2017-07-25 22:14:08,121 INFO     rpmlb.app                  Building...
HELLO before build
HELLO during package1 build
HELLO during package2 build
2017-07-25 22:14:08,138 INFO     rpmlb.app                  Done successfully.

custom-env-var-hooks.txt custom-env-var-recipe.txt

hroncok commented 7 years ago

Well I assume it was never intended as a feature, rather as better safe than sorry approach when changing the locale in here, although the code replaced suggested it was there even before.