Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ Assignee
	Description • 10 years ago

This is being broken out of bug 1132760, which will be to copy PR_DuplicateEnvironment into upstream NSPR.  Once it's migrated back downstream into mozilla-central, the copy in process_util_linux and mozglue can be removed, and the NSPR version used on all Linux platforms.

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ Assignee
	Comment 1 • 10 years ago

This patch is for use after the patch in bug 1132760 lands in NSPR and makes its way into nsprpub.  I might also need an actual IPC peer for this.

	Dave Hylands [:dhylands]
	Comment 2 • 10 years ago

Comment on attachment 8571776 [details] [diff] [review]
bug1133073-dupenv-from-nspr-hg4.diff

Review of attachment 8571776 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me. Thanks for doing this.

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ Assignee
	Comment 3 • 9 years ago

I just realized: Solaris doesn't use process_util_linux.cc.  That means this can actually land now (modulo whatever rebasing it needs), because NSPR_MINVER is already up to NSPR 4.11, which has PR_DuplicateEnvironment everywhere except Solaris (where bug 1209397 means it's not exported until 4.12).

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ Assignee
	Comment 4 • 9 years ago

Rebase patch to current m-c; carrying over r+.

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ Assignee
	Comment 5 • 9 years ago

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bcc2a251c0b (tests chosen are slightly arbitrary; goal is to make sure child processes aren't entirely broken).

	Pulsebot
	Comment 6 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b4bffb2ce7c

	Pulsebot
	Comment 7 • 9 years ago

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a87da113e0ab

	Carsten Book [:Tomcat]
	Comment 8 • 9 years ago

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=18826447&repo=mozilla-inbound

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ Assignee
	Comment 9 • 9 years ago

In hindsight, I agree that I should have run Try on Android as well — it doesn't do content e10s like desktop and B2G, but it does use IPC, and it is affected by the PR_{Get,Set}Env interceptions.

But I'm at a loss as to how my patch caused that particular build breakage — “error: cannot find -lc”, “error: cannot open crtend_android.o: No such file or directory”, etc. suggest a problem with a linker path, and I wasn't directly doing anything to any of those — and it affects one of the executables from the NSS build, which the patch doesn't go anywhere near.  (Also… while one can argue whether the tool that makes those weird FIPS library signatures is ever useful, it's markedly less useful when compiled for the target rather than the host, as it seems to be here.  But that's probably beside the point.)

In any case, I've pushed to Try to check if it's reproducible: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56e1192466b0

	Carsten Book [:Tomcat]
	Comment 10 • 9 years ago

seems its reproducible,same error in the try run somehow :(

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ Assignee
	Comment 11 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69fcf055fce2

glandium, that $(SHARED_LIBS:$(DEPTH)%=$(MOZ_BUILD_ROOT)%) you added to config/external/nss/Makefile.in in bug 1077148 seems to have become load-bearing for the Android build, such that it breaks (see treeherder links in comment #8, comment #9) if MOZ_GLUE_WRAP_LDFLAGS is empty.  Any suggestions?

	Mike Hommey [:glandium]
	Comment 12 • 9 years ago

(In reply to Jed Davis [:jld] from comment #11)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=69fcf055fce2
> 
> glandium, that $(SHARED_LIBS:$(DEPTH)%=$(MOZ_BUILD_ROOT)%) you added to
> config/external/nss/Makefile.in in bug 1077148 seems to have become
> load-bearing for the Android build, such that it breaks (see treeherder
> links in comment #8, comment #9) if MOZ_GLUE_WRAP_LDFLAGS is empty.  Any
> suggestions?

Make it conditional to MOZ_GLUE_WRAP_LDFLAGS being non empty or ANDROID being set. You can replace the second complex conditional with a check on whether NSS_EXTRA_LDFLAGS is empty.

	Pulsebot
	Comment 13 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f6bb9f7e60d

	Pulsebot
	Comment 14 • 9 years ago

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18f5d9d83ab8

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ Assignee
	Comment 15 • 9 years ago

So that burned the tests with a lot of errors like this:

libsoftokn3.so: Relocation to NULL @0x00024e10 for symbol "__wrap_PR_GetEnv"

Looking at the logs, I can see some linker steps still using -Wl,--wrap=PR_GetEnv,--wrap=PR_SetEnv even though it's no longer in the source tree.  Which of course didn't happen on Try.  My guess is that there's some kind of issue with subprojects with their own configury or build systems (I noticed some of the problematic linker command lines where for things in js/) where touching the toplevel configure.in isn't enough to reconfigure them (or equivalent), and I'll need to do a full clobber instead.

(Treeherder link for reference: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7f6bb9f7e60d )

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ Assignee
	Comment 16 • 9 years ago

A few failures on https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a9f906890d9 but they look like they're not due to this patch.

	Pulsebot
	Comment 17 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/f75b461c2c3c

	Carsten Book [:Tomcat]
	Comment 18 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/f75b461c2c3c