Page MenuHomeSoftware Heritage
Differential D5146

Use minimal environment in Mercurial loader
ClosedPublic
Actions

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

Details

Reviewers
vlorentz
Group Reviewers
Reviewers
Commits
rDLDHGc74e5791b93e: Use minimal environment in Mercurial loader
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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Alphare created this revision.Feb 25 2021, 3:56 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptFeb 25 2021, 3:56 PM
swh-public-ci added a comment.Feb 25 2021, 3:58 PM

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!
Alphare updated this revision to Diff 18408.Feb 25 2021, 4:03 PM

Fix missing PATH handling

swh-public-ci added a comment.Feb 25 2021, 4:05 PM

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.

Harbormaster completed remote builds in B19486: Diff 18408.Feb 25 2021, 4:05 PM
Alphare requested review of this revision.Feb 25 2021, 4:05 PM
vlorentz accepted this revision.Feb 26 2021, 8:54 AM
vlorentz added a subscriber: vlorentz.

thx

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
Alphare added a comment.Feb 26 2021, 11:00 AM
In D5146#130314, @vlorentz wrote:

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!

Alphare updated this revision to Diff 18431.Feb 26 2021, 11:26 AM

Rebase

swh-public-ci added a comment.Feb 26 2021, 11:28 AM

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.

Harbormaster completed remote builds in B19510: Diff 18431.Feb 26 2021, 11:28 AM
Alphare updated this revision to Diff 18437.Feb 26 2021, 12:19 PM

Rebase

swh-public-ci added a comment.Feb 26 2021, 12:21 PM

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.

Harbormaster completed remote builds in B19516: Diff 18437.Feb 26 2021, 12:21 PM
vlorentz accepted this revision.Feb 26 2021, 4:40 PM

meh

This revision is now accepted and ready to land.Feb 26 2021, 4:40 PM
Alphare added a comment.Feb 26 2021, 4:43 PM
In D5146#130548, @vlorentz wrote:

meh

Agreed

ardumont added a subscriber: ardumont.Feb 26 2021, 4:46 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).

¯\_(ツ)_/¯

Alphare updated this revision to Diff 18456.Feb 26 2021, 4:49 PM

Rebase

swh-public-ci added a comment.Feb 26 2021, 4:51 PM

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.

Harbormaster completed remote builds in B19536: Diff 18456.Feb 26 2021, 4:51 PM
Closed by commit rDLDHGc74e5791b93e: Use minimal environment in Mercurial loader (authored by Alphare). · Explain WhyFeb 26 2021, 4:51 PM
This revision was automatically updated to reflect the committed changes.
Alphare added a commit: rDLDHGc74e5791b93e: Use minimal environment in Mercurial loader.

Revision Contents
Changeset List

PathSize
swh/
loader/
mercurial/
17 lines
6 lines
17 lines

Diff 18457

View Options

swh/loader/mercurial/identify.py

Loading...
View Options

swh/loader/mercurial/loader.py

Loading...
View Options

swh/loader/mercurial/utils.py

Loading...
About · Developer information · Legal