Closed
Bug 1399100
Opened 8 years ago
Closed 8 years ago
Use a temporary Firefox-only patch on top of NSPR, to avoid exposing a temporary function
Categories
(Firefox Build System :: General, enhancement)
Tracking
(firefox57 fixed)
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: KaiE, Assigned: KaiE)
References
Details
Attachments
(3 files, 2 obsolete files)
470 bytes,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
4.26 KB,
patch
|
Details | Diff | Splinter Review | |
1.84 KB,
patch
|
Details | Diff | Splinter Review |
Bug 1384633 had introduced a new API to NSPR.
We came to the conclusion that API isn't ideal and should be temporary.
Also, the API is supposed to be Windows-only.
I think it's not ideal that we introduce a new publicly exported symbol to a new NSPR release, even if it's just a temporary symbol.
I suggest these changes:
- in the official NSPR release, in nspr.def,
don't export PR_EXPERIMENTAL_ONLY_IN_4_17_GetOverlappedIOHandle
- use a downstream patch in Firefox code, that adds the
export to nspr.def
I'll attach two patches, that make these changes to NSPR and to Firefox.
If accepted, I offer to land both changes into mozilla-inbound at the same time.
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Target Milestone: mozilla56 → mozilla57
Version: 56 Branch → 57 Branch
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8907048 -
Attachment description: temporary patch for firefox, including documentation with removal plan → temporary patch for firefox 57, including documentation with removal plan
Assignee | ||
Comment 3•8 years ago
|
||
Note the temporary patch also marks the symbol with version tag NSPR_4.17_fork to make it clear it's not an official API.
Assignee | ||
Comment 4•8 years ago
|
||
I cannot imagine problems with this approach, but let's be certain, try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=928472e9fc41fdb161fb6c3573852e176d824e11
Assignee | ||
Comment 5•8 years ago
|
||
I don't see any build failures or networking failures in the try build, only intermittent failures, that seem unrelated to this change.
Assignee | ||
Updated•8 years ago
|
Attachment #8907044 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•8 years ago
|
Attachment #8907048 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 6•8 years ago
|
||
Patrick, does the suggestion sound ok to you? Unfortunately both Dragana and Honza seem to be away currently, and the freeze for this NSPR release is supposed to be this week. Sorry for doing this cleanup late.
Flags: needinfo?(mcmanus)
Updated•8 years ago
|
Attachment #8907044 -
Flags: review?(mh+mozilla) → review+
Comment 8•8 years ago
|
||
Comment on attachment 8907048 [details] [diff] [review]
temporary patch for firefox 57, including documentation with removal plan
Review of attachment 8907048 [details] [diff] [review]:
-----------------------------------------------------------------
::: nsprpub/patches/README.TXT
@@ +1,1 @@
> +bug1399100-temporary.patch :
A readme is not going to be noticed by someone who runs client.py to update nspr. You should put this in client.py output, or make client.py apply the patch.
Attachment #8907048 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #8)
>
> A readme is not going to be noticed by someone who runs client.py to update
> nspr. You should put this in client.py output, or make client.py apply the
> patch.
You are right that we need to add some protection.
I'm reluctant to introduce a mechanism that's fully automatic, as I don't want to encourage people to use this approach routinely.
Therefore I prefer a warning. The attached new patch moves the patches to a separate directory, which won't get removed by the update script. If there are any files inside the directory nsprpub.patches (when updating nspr) or inside the directory security/nss.patches (when updating nss), the script will print a warning, and require to press enter, allowing the operator of the script to notice.
Assignee | ||
Comment 10•8 years ago
|
||
Attachment #8907048 -
Attachment is obsolete: true
Attachment #8908033 -
Flags: review?(mh+mozilla)
Comment 11•8 years ago
|
||
Comment on attachment 8908033 [details] [diff] [review]
1399100-firefox-temporary-v2.patch
Review of attachment 8908033 [details] [diff] [review]:
-----------------------------------------------------------------
::: client.py
@@ +119,5 @@
>
> +def warn_if_patch_exists(path):
> + # If the given patch directory exists and contains at least one file,
> + # then print warning and wait for the user to acknowledge.
> + if os.path.exists(path) and os.path.isdir(path):
you only need the latter.
@@ +120,5 @@
> +def warn_if_patch_exists(path):
> + # If the given patch directory exists and contains at least one file,
> + # then print warning and wait for the user to acknowledge.
> + if os.path.exists(path) and os.path.isdir(path):
> + for root, dirs, files in os.walk(path):
You can just do something like if os.listdir(path)
::: nsprpub.patches/README.TXT
@@ +1,1 @@
> +bug1399100-temporary.patch :
Adding a directory at the toplevel for this is not really nice. The previous patch had a subdirectory of nsprpub, that's better.
Attachment #8908033 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #11)
>
> ::: nsprpub.patches/README.TXT
> @@ +1,1 @@
> > +bug1399100-temporary.patch :
>
> Adding a directory at the toplevel for this is not really nice. The previous
> patch had a subdirectory of nsprpub, that's better.
But that subdirectory will be removed by the client.py update action, so the user cannot easily get access to the previous patch, without digging it out from history.
Assignee | ||
Comment 13•8 years ago
|
||
(In reply to Kai Engert (:kaie:) from comment #12)
>
> But that subdirectory will be removed by the client.py update action, so the
> user cannot easily get access to the previous patch, without digging it out
> from history.
OK, I could change client.py to move the patch files to a temporary location, and then move them back to the subdirectory after the update has completed.
Assignee | ||
Comment 14•8 years ago
|
||
Thanks for your suggestions, I've addressed them.
Do you think this could land as is it is now, and have additional cleanup/tweaks done later? It would be great to get this landed and NSPR released prior to the next merge day. Thanks!
Attachment #8908033 -
Attachment is obsolete: true
Attachment #8908563 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 15•8 years ago
|
||
(Apparently merge day was last night. We'll have to ask for beta approval to get this landed.)
Assignee | ||
Comment 16•8 years ago
|
||
Franziskus gave me r+ on IRC.
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8907044 [details] [diff] [review]
remove the symbol from official NSPR
https://hg.mozilla.org/projects/nspr/rev/64f9cb0f8b1c
Assignee | ||
Comment 18•8 years ago
|
||
Comment on attachment 8908563 [details] [diff] [review]
1399100-firefox-temporary-v3.patch
NSPR part:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63281179bfe0
client.py:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f71235ab648
Assignee | ||
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment 19•8 years ago
|
||
bugherder |
Updated•8 years ago
|
Attachment #8908563 -
Flags: review?(mh+mozilla)
Comment 20•8 years ago
|
||
This breaks NSS uplifts. I'll attach a patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 21•8 years ago
|
||
NSS doesn't have a patch folder.
Attachment #8910146 -
Flags: review?(kaie)
Assignee | ||
Comment 22•8 years ago
|
||
Comment on attachment 8910146 [details] [diff] [review]
fix-client.py.patch
The first part looks good.
The second part needs a change. Because you have moved the permanent dir, it no longer exists, and we need to check for the temporary location.
# move patch directory back to a subdirectory
- shutil.move(temporary_patch_dir, permanent_patch_dir)
+ if os.path.exists(temporary_patch_dir):
+ shutil.move(temporary_patch_dir, permanent_patch_dir)
If you agree, r=kaie with that change.
Attachment #8910146 -
Flags: review?(kaie)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(franziskuskiefer)
Comment 23•8 years ago
|
||
> The second part needs a change. Because you have moved the permanent dir, it no longer exists, and we need to check for the temporary location.
oops, indeed. I'll change that and land it.
Flags: needinfo?(franziskuskiefer)
Comment 24•8 years ago
|
||
Pushed by franziskuskiefer@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76aceb596872
fix client.py follow-up, r=kaie
Comment 25•8 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•