PR#414: Revert a temporary fix for mock >= 1.4.1 of year 2017 - FedoraReview

FedoraReview

#414 Revert a temporary fix for mock >= 1.4.1 of year 2017

Merged 2 years ago by ngompa. Opened 3 years ago by sergiomb.

sergiomb/FedoraReview bootstrap-chroot into master

Revert a temporary fix for mock >= 1.4.1 of 2017

Sérgio M. Basto • 2 years ago

48780d2

fedora-review.1

file modified

+1 -2

		`@@ -123,8 +123,7 @@`
		`.TP 4`
		`.B -o, --mock-options "options..."`
		`Mock options for the build. Defaults to --no-cleanup-after --no-clean,`
		`- for use with mock >= 1.4.1 --no-bootstrap-chroot additionally, you might`
		`- want this along with other options you provide.`
		`+ you might want this along with other options you provide.`
		`.TP 4`
		`.B --no-report`
		`Do not generate the review template.`

src/FedoraReview/settings.py

file modified

-2

		`@@ -74,8 +74,6 @@`
		`def _mock_options_default():`
		`""" Returns the default mock options """`
		`mock_opts = "--no-cleanup-after --no-clean"`
		`- if version.parse(_check_mock_ver()) >= version.parse("1.4.1"):`
		`- mock_opts = "--no-bootstrap-chroot %s" % mock_opts`
		`return mock_opts`

sergiomb commented 3 years ago

after first comit approved in the other PR , I'd like revert this temporary fix, to have a faster review

sergiomb commented 2 years ago

why this PR is ignored ?

sergiomb commented 2 years ago

this is fix a nasty bug , we shouldn't use --no-bootstrap-chroot , all builds take much longer

ngompa commented 2 years ago

Sorry, the reason I haven't merged this is because I don't know what this is about. There is no explanation about what this is and what it's for.

ngompa commented 2 years ago

Also, if we remove --no-bootstrap-chroot, we're going back to running two chroot setup cycles (one bootstrap and another regular), which would make this take longer.

rebased onto 48780d2

2 years ago

sergiomb commented 2 years ago

After review PR #413 , IIRC I notice this one , IIRC we shouldn't change default configuration , the original commit said temporary fix , which we don't need anymore I think , but I have to review it again

sergiomb commented 2 years ago

I remember that this patch increased revision time, but in today's tests it didn't, I think it's because today the mirror and the network are very fast..., but I don't see any utility in keeping the temporary fix, since the default settings of mock also work well

I tested with bug 1976038 (golang-github-sgreben-flagvar) [1] this time on first attempt with patch have bad results, but on second try I have much better results.

[1]

fedora-review -v -b 1976038
cp ~/.cache/fedora-review.log  fedora-review_with_patch.log

#removed the patch 
fedora-review -v -b 1976038
cp ~/.cache/fedora-review.log  fedora-review_without_patch.log
diff <(cut -b13- fedora-review_without_patch.log) <(cut -b13- fedora-review_with_patch.log) -up > test1.txt

#readd the patch
fedora-review -v -b 1976038
cp ~/.cache/fedora-review.log  fedora-review_with_patch_try2.log
diff <(cut -b13- fedora-review_without_patch.log) <(cut -b13- fedora-review_with_patch_try2.log) -up > teste2.txt

ngompa commented 2 years ago

@msuchy, @praiskup: what do you think?

sergiomb commented 2 years ago

https://sergiomb.fedorapeople.org/test1.txt
https://sergiomb.fedorapeople.org/test2.txt

So I guest it will need much less internet bandwidth

msuchy commented 2 years ago

The chroots are cached. So after the first round, the overhead is close to zero. And it can save you a lot of headaches when new incompatibilities land in rawhide (mainly rpm).

sergiomb commented 2 years ago

@msuchy thank you for opinion , I think it is approved , please merge it , I had tested last 4 months ...

Pull-Request has been merged by ngompa

2 years ago