Closed Bug 1399100 Opened 3 years ago Closed 3 years ago

Use a temporary Firefox-only patch on top of NSPR, to avoid exposing a temporary function

Categories

(Firefox Build System :: General, enhancement)

57 Branch
enhancement
Not set
normal

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: KaiE, Assigned: KaiE)

References

Details

Attachments

(3 files, 2 obsolete files)

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.
Target Milestone: mozilla56 → mozilla57
Version: 56 Branch → 57 Branch
Attachment #8907048 - Attachment description: temporary patch for firefox, including documentation with removal plan → temporary patch for firefox 57, including documentation with removal plan
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.
I cannot imagine problems with this approach, but let's be certain, try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=928472e9fc41fdb161fb6c3573852e176d824e11
See Also: → 1384633
I don't see any build failures or networking failures in the try build, only intermittent failures, that seem unrelated to this change.
Attachment #8907044 - Flags: review?(mh+mozilla)
Attachment #8907048 - Flags: review?(mh+mozilla)
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)
sgtm. thanks kai!
Flags: needinfo?(mcmanus)
Attachment #8907044 - Flags: review?(mh+mozilla) → review+
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)
(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.
Attachment #8907048 - Attachment is obsolete: true
Attachment #8908033 - Flags: review?(mh+mozilla)
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)
(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.
(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.
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)
(Apparently merge day was last night. We'll have to ask for beta approval to get this landed.)
Franziskus gave me r+ on IRC.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Attachment #8908563 - Flags: review?(mh+mozilla)
This breaks NSS uplifts. I'll attach a patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
NSS doesn't have a patch folder.
Attachment #8910146 - Flags: review?(kaie)
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)
Flags: needinfo?(franziskuskiefer)
> 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)
https://hg.mozilla.org/mozilla-central/rev/76aceb596872
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.