Page MenuHomeSoftware Heritage

Use minimal environment in Mercurial loader
ClosedPublic

Authored by Alphare on Feb 25 2021, 3:56 PM.

Details

Summary

This generalizes the work done in ef3a2ba79ea9 to (supposedly) all
places invoking Mercurial. In short, this limits the environment to the
smallest subset needed (i.e. $PATH) and uses the Mercurial-specific
variables to disable user customizations and configs.

Diff Detail

Repository
rDLDHG Mercurial loader
Branch
minimal-env
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 19484
Build 30224: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 30223: arc lint + arc unit

Unit TestsFailed

TimeTest
50 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.loader.mercurial.tests.test_loader::test_loader_hg_new_visit_no_release
swh_storage = <swh.storage.filter.FilteringProxyStorage object at 0x7fdb89196198> datadir = '/var/lib/jenkins/workspace/DLDHG/tests-on-diff/.tox/py3/lib/python3.7/site-packages/swh/loader/mercurial/tests/data' tmp_path = PosixPath('/tmp/pytest-of-jenkins/pytest-0/test_loader_hg_new_visit_no_re1')
50 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.loader.mercurial.tests.test_loader::test_loader_hg_new_visit_with_release
swh_storage = <swh.storage.filter.FilteringProxyStorage object at 0x7fdb891c5400> datadir = '/var/lib/jenkins/workspace/DLDHG/tests-on-diff/.tox/py3/lib/python3.7/site-packages/swh/loader/mercurial/tests/data' tmp_path = PosixPath('/tmp/pytest-of-jenkins/pytest-0/test_loader_hg_new_visit_with_1')
64 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.loader.mercurial.tests.test_loader::test_visit_error_with_clone_error
swh_storage = <swh.storage.filter.FilteringProxyStorage object at 0x7fdb891eb128> datadir = '/var/lib/jenkins/workspace/DLDHG/tests-on-diff/.tox/py3/lib/python3.7/site-packages/swh/loader/mercurial/tests/data' tmp_path = PosixPath('/tmp/pytest-of-jenkins/pytest-0/test_visit_error_with_clone_er0')
50 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.loader.mercurial.tests.test_loader::test_visit_error_with_snapshot_partial
swh_storage = <swh.storage.filter.FilteringProxyStorage object at 0x7fdb89123f28> datadir = '/var/lib/jenkins/workspace/DLDHG/tests-on-diff/.tox/py3/lib/python3.7/site-packages/swh/loader/mercurial/tests/data' tmp_path = PosixPath('/tmp/pytest-of-jenkins/pytest-0/test_visit_error_with_snapshot0')
59 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.loader.mercurial.tests.test_loader::test_visit_error_with_status_not_found[404: NOT FOUND]
swh_storage = <swh.storage.filter.FilteringProxyStorage object at 0x7fdb8923e320> datadir = '/var/lib/jenkins/workspace/DLDHG/tests-on-diff/.tox/py3/lib/python3.7/site-packages/swh/loader/mercurial/tests/data' tmp_path = PosixPath('/tmp/pytest-of-jenkins/pytest-0/test_visit_error_with_status_n2')
View Full Test Results (12 Failed · 31 Passed)

Event Timeline

Build has FAILED

Patch application report for D5146 (id=18406)

Rebasing onto d8572187e9...

Current branch diff-target is up to date.
Changes applied before test
commit 99df13c6cb912cb272c5a84c8ad6cfd79576dfb5
Author: Raphaël Gomès <rgomes@octobus.net>
Date:   Thu Feb 25 15:51:24 2021 +0100

    Use minimal environment in Mercurial loader
    
    This generalizes the work done in ef3a2ba79ea9 to (supposedly) all
    places invoking Mercurial. In short, this limits the environment to the
    smallest subset needed (i.e. `$PATH`) and uses the Mercurial-specific
    variables to disable user customizations and configs.

Link to build: https://jenkins.softwareheritage.org/job/DLDHG/job/tests-on-diff/165/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDHG/job/tests-on-diff/165/console

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 25 2021, 3:58 PM
Harbormaster failed remote builds in B19484: Diff 18406!

Build is green

Patch application report for D5146 (id=18408)

Rebasing onto d8572187e9...

Current branch diff-target is up to date.
Changes applied before test
commit c7999db35929e599a0ecff5eb77a16d38818a8d7
Author: Raphaël Gomès <rgomes@octobus.net>
Date:   Thu Feb 25 15:51:24 2021 +0100

    Use minimal environment in Mercurial loader
    
    This generalizes the work done in ef3a2ba79ea9 to (supposedly) all
    places invoking Mercurial. In short, this limits the environment to the
    smallest subset needed (i.e. `$PATH`) and uses the Mercurial-specific
    variables to disable user customizations and configs.

See https://jenkins.softwareheritage.org/job/DLDHG/job/tests-on-diff/167/ for more details.

This revision is now accepted and ready to land.Feb 26 2021, 8:54 AM
vlorentz requested changes to this revision.EditedFeb 26 2021, 8:55 AM

hmm actually, modifying this global may interact with other parts of the code in surprising way.

Could you only use the env argument of subprocess functions instead of changing os.environ?

This revision now requires changes to proceed.Feb 26 2021, 8:55 AM

hmm actually, modifying this global may interact with other parts of the code in surprising way.

Could you only use the env argument of subprocess functions instead of changing os.environ?

That was my thinking also, but it looks like billiard.Process is just Python's Thread in disguise and neither it nor Queue provide a way of setting the environment. Maybe I'm missing something, I'd be glad not to touch global state!

Build is green

Patch application report for D5146 (id=18431)

Rebasing onto 1dbb8474ad...

Current branch diff-target is up to date.
Changes applied before test
commit 30d0c36d0ac84b93aad591d06f6c01b721941ee1
Author: Raphaël Gomès <rgomes@octobus.net>
Date:   Thu Feb 25 15:51:24 2021 +0100

    Use minimal environment in Mercurial loader
    
    This generalizes the work done in ef3a2ba79ea9 to (supposedly) all
    places invoking Mercurial. In short, this limits the environment to the
    smallest subset needed (i.e. `$PATH`) and uses the Mercurial-specific
    variables to disable user customizations and configs.

See https://jenkins.softwareheritage.org/job/DLDHG/job/tests-on-diff/173/ for more details.

Build is green

Patch application report for D5146 (id=18437)

Rebasing onto 04a1213f36...

Current branch diff-target is up to date.
Changes applied before test
commit c05d78bef086325a5697be926dc108a6c975f9f2
Author: Raphaël Gomès <rgomes@octobus.net>
Date:   Thu Feb 25 15:51:24 2021 +0100

    Use minimal environment in Mercurial loader
    
    This generalizes the work done in ef3a2ba79ea9 to (supposedly) all
    places invoking Mercurial. In short, this limits the environment to the
    smallest subset needed (i.e. `$PATH`) and uses the Mercurial-specific
    variables to disable user customizations and configs.

See https://jenkins.softwareheritage.org/job/DLDHG/job/tests-on-diff/175/ for more details.

This revision is now accepted and ready to land.Feb 26 2021, 4:40 PM

meh

Agreed

I don't have anything better to propose.

Still, it's sad because i don't like those kind of side effects
in the constructor (env variables manipulation).

¯\_(ツ)_/¯

Build is green

Patch application report for D5146 (id=18456)

Rebasing onto 5b8da68a92...

Current branch diff-target is up to date.
Changes applied before test
commit c74e5791b93ef92ac3688c688de3b28f8e18a2d0
Author: Raphaël Gomès <rgomes@octobus.net>
Date:   Thu Feb 25 15:51:24 2021 +0100

    Use minimal environment in Mercurial loader
    
    This generalizes the work done in ef3a2ba79ea9 to (supposedly) all
    places invoking Mercurial. In short, this limits the environment to the
    smallest subset needed (i.e. `$PATH`) and uses the Mercurial-specific
    variables to disable user customizations and configs.

See https://jenkins.softwareheritage.org/job/DLDHG/job/tests-on-diff/179/ for more details.