datasig-ac-uk / esig

esig python package
https://esig.readthedocs.io/en/latest/
GNU General Public License v3.0
42 stars 3 forks source link

Recombine switch n #92

Closed inakleinbottle closed 3 years ago

inakleinbottle commented 3 years ago

Implemented switch to disable inclusion of recombine when an environmental variable is not set.

inakleinbottle commented 3 years ago

We still need to introduce the environmental variable to force building with recombine inside the build system.

rolyp commented 3 years ago

@inakleinbottle Thanks Sam. Does this version end up excluding recombine, because ESIG_WITH_RECOMBINE is always unset in os.environment? If so, I suggest adding one more commit that ensures ESIG_WITH_RECOMBINE is initially set to some value. Then the existing behaviour is preserved exactly by the pull request, which is preferable to merging something that drops out recombine. It will also mean you will have validated the behaviour for case when ESIG_WITH_RECOMBINE is defined.

Then I’ll create another pull request to remove that default value of ESIG_WITH_RECOMBINE and instead set it explicitly in each build.

inakleinbottle commented 3 years ago

@rolyp My intention was to set the environment variable in the setup of the environment. The hope is that when building here, recombine is included as intended, but if a user downloads the source to compile then it will not be set and so they can build without recombine. I was hoping you could suggest the most appropriate place to set such a variable.

rolyp commented 3 years ago

@inakleinbottle Ok, I see. My suggestion was to initially set this variable once to some value just before you read it (essentially, introducing the feature but not really using it), and leave it to me to split that single bit of information into one bit per build. But if you want jump straight to the latter setup, I suggest putting that information in to each job inside build.yml, i.e.:

export ESIG_WITH_RECOMBINE=true

at the beginning of each Mac/Linux run step, and $env:ESIG_WITH_RECOMBINE=true (which I think is the Powershell equivalent) at the beginning of each Windows run step.

As an additional validation of the conditional logic, I can then merge build-failing.yml.disabled (which contains the 32-bit Windows builds for Python 3.5, 3.8 and 3.9) into build.yml, but in those cases omit to set the flag. In theory those should build/test ok, albeit without recombine.

inakleinbottle commented 3 years ago

Well I managed think I've finally managed to get this working. I set the environment variable in the build.yml, and for the Linux build I added an environment variable passed through into the docker. I'm not sure if this is terribly robust.

inakleinbottle commented 3 years ago

@rolyp This seems to be working now. I think we're ready to merge this now.

rolyp commented 3 years ago

@inakleinbottle Great. Good work on spotting the Docker --env command. I think that looks reasonable for now, I’ll merge this and then try it out with the failing Windows builds.