Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use version-specific prefs files when running Firefox #10443

Merged
merged 1 commit into from
Apr 12, 2018

Conversation

jgraham
Copy link
Contributor

@jgraham jgraham commented Apr 12, 2018

Firefox requires a prefs file to be loaded to ensure that it doesn't
make external network connections, and to make sure that other
settings are appropriate for testing. Previously we always used the
version of the prefs file from master, which is usually fine for
nightly builds, but doesn't work with release builds if it happens
that a pref changed.

This change uses the correct release version of the prefs file for
firefox releases and beta versions. It continues to use the master
version for any nightly build; this could be fixed in some cases if we
are able to access the actual commit hash used to build Firefox, but
probably isn't too bad an approximation.

The caching algorithm was changed so that release versions of the
prefs are cached forever, and the nightly version is updated once per
day (although this doesn't quite match the nightly release cadence,
it's only going to fail in edge cases where the prefs were changed in
the file, but the nightly version was not yet updated, of vice-versa.)


This change is Reviewable

Copy link
Contributor

@jugglinmike jugglinmike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, James! Just a few small requests

return None, "nightly"
version, status = m.groups()
channel = {"a": "nightly", "b": "beta", None: "stable"}
return version, channel.get(status, "stable")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know which of the two approaches to defining the default value is more Pythonic, but I don't think we need them both.

else:
# Always use tip as the tag for nightly; this isn't quite right
# but to do better we need the actual build revision, which we
# can get if we have an application.ini file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hint for future improvement is appreciated!

f.write(resp.content)
else:
print("Using cached test prefs from %s" % cache_file)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a logger instance in this file. Mind using that instead?

@@ -1,4 +1,5 @@
import logging
import glob
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line may be removed

Firefox requires a prefs file to be loaded to ensure that it doesn't
make external network connections, and to make sure that other
settings are appropriate for testing. Previously we always used the
version of the prefs file from master, which is usually fine for
nightly builds, but doesn't work with release builds if it happens
that a pref changed.

This change uses the correct release version of the prefs file for
firefox releases and beta versions. It continues to use the master
version for any nightly build; this could be fixed in some cases if we
are able to access the actual commit hash used to build Firefox, but
probably isn't too bad an approximation.

The caching algorithm was changed so that release versions of the
prefs are cached forever, and the nightly version is updated once per
day (although this doesn't quite match the nightly release cadence,
it's only going to fail in edge cases where the prefs were changed in
the file, but the nightly version was not yet updated, of vice-versa.)
@jgraham
Copy link
Contributor Author

jgraham commented Apr 12, 2018

I fixed everything apart from the logger thing, which seems to be a little complicated. I need to remember what we do to make the stdlib logger work from mozlog, because at the moment it's looking pretty broken to me, but then it works later so I guess I'm missing something.

I think we should do that in a followup since it's not really a change in this PR (we were using print before these changes as well).

@jugglinmike
Copy link
Contributor

I think we should do that in a followup since it's not really a change in this PR (we were using print before these changes as well).

Works for me. Could you add a link here when you do?

Copy link
Contributor

@jugglinmike jugglinmike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally with Firefox version 59.0.2 and Firefox Nightly. Prior to application of the patch, version 59.0.2 consistently fails due to its making network requests on startup. After applying the patch, these requests are correctly disabled for both versions.

Thanks, @jgraham!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants