Closed Bug 1442791 Opened 6 years ago Closed 6 years ago

gfx/angle/checkout/src/common/system_utils.cpp: undefined reference to `angle::GetPathSeparator()'

Categories

(Core :: Graphics, defect, P3)

Product:
Core
Component:
Graphics
Platform:
Unspecified
FreeBSD
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 + fixed
firefox61 --- fixed

People

(Reporter: jbeich, Assigned: jgilbert)

References

Details

(Keywords: regression, Whiteboard: gfx-noted)

Attachments

(1 file, 3 obsolete files)

Bug 1442791 - Build _linux files on BSDs in ANGLE. -
59 bytes, text/x-review-board-request
jrmuizel
: review+
jbeich
: feedback+
jcristau
: approval-mozilla-beta+
Details 
$ c++ -v
FreeBSD clang version 4.0.0 (tags/RELEASE_400/final 297347) (based on LLVM 4.0.0)
Target: x86_64-unknown-freebsd11.1
Thread model: posix
InstalledDir: /usr/bin

$ echo "ac_add_options --disable-webrtc # bug 1437670 workaround" >>.mozconfig
$ ./mach bootstrap # select Firefox for Desktop
$ ./mach build
[...]
../../gfx/angle/targets/angle_common/system_utils.o: In function `angle::PrependPathToEnvironmentVar(char const*, char const*)':
gfx/angle/checkout/src/common/system_utils.cpp:(.text+0x1b): undefined reference to `angle::GetEnvironmentVar(char const*)'
gfx/angle/checkout/src/common/system_utils.cpp:(.text+0x54): undefined reference to `angle::GetPathSeparator()'
gfx/angle/checkout/src/common/system_utils.cpp:(.text+0x9d): undefined reference to `angle::SetEnvironmentVar(char const*, char const*)'
/usr/local/bin/ld: libxul.so: hidden symbol `_ZN5angle16GetPathSeparatorEv' isn't defined
/usr/local/bin/ld: final link failed: Bad value
Looks like BSD should also include system_utils_linux.cpp.
No longer blocks: angle-60
Depends on: angle-60
(In reply to Jeff Gilbert [:jgilbert] from comment #1)
> Looks like BSD should also include system_utils_linux.cpp.

With linux-only code such as 

    ssize_t result = readlink("/proc/self/exe", path, sizeof(path) - 1);

?

Ew.

I suppose that means hacking https://dxr.mozilla.org/mozilla-central/source/gfx/angle/src/angle.gyp#164 and/or https://dxr.mozilla.org/mozilla-central/source/gfx/angle/src/angle.gyp#274 and/or all the is_linux conditions in https://dxr.mozilla.org/mozilla-central/source/gfx/angle/BUILD.gn ?
A few issues remain:
- update-angle.py is untested, requires Windows or I couldn't figure out how to use it (README_MOZILLA is missing)
- _linux.cpp is shared with BSDs to avoid copypasta (both are ELF platforms)
  and support /proc/self/exe but under /emul/linux (NetBSD) or /compat/linux (FreeBSD)
- OpenBSD dropped /proc in 5.7 and never supported KERN_PROC_PATHNAME
- Upstreaming is complicated because of Google Account walled garden; I don't care
  about credit, so feel free to forward the changes as your own
Gecko doesn't use angle::GetExecutablePath() or angle::GetExecutableDirectory(), so OpenBSD can get away with a broken one. ;)
https://searchfox.org/mozilla-central/search?regexp=true&q=%5CbGetExecutable(Path%7CDirectory)
Attached patch Smallest fix (alternative) (obsolete) — — Splinter Review 
Instead of touching ANGLE code and complicating rebases an alternative option is to simply introduce a fallback. Not clue how to translate this into gfx/angle/update-angle.py.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d83c09ff62482fbc240259d27d12d7b193174970
Attachment #8955834 - Flags: review?(jgilbert)
Fwiw, the 'smallest fix' would work for me. Simpler, smaller, smarter :)
Comment on attachment 8955834 [details] [diff] [review]
Smallest fix (alternative)

This isn't what we want. (It doesn't really fit fith efforts to make and keep this programatic)
Attachment #8955834 - Flags: review?(jgilbert) → review-
(In reply to Jan Beich from comment #6)
> Gecko doesn't use angle::GetExecutablePath() or
> angle::GetExecutableDirectory(), so OpenBSD can get away with a broken one.
> ;)
> https://searchfox.org/mozilla-central/
> search?regexp=true&q=%5CbGetExecutable(Path%7CDirectory)

I'll look into if we can alter the target dependencies here.
Assignee: nobody → jgilbert
Priority: -- → P3
Whiteboard: gfx-noted
So, which way should this get fixed ? can we help there ?
[Tracking Requested - why for this release]: Bug 1440849 landed after soft freeze, destabilizing Tier3 support (DragonFly, FreeBSD, NetBSD, OpenBSD, Solaris). No known workaround as ANGLE cannot be disabled during build.

https://www.mail-archive.com/dev-platform@lists.mozilla.org/msg24915.html
tracking-firefox60: --- → ?
Blocks: angle-60
No longer depends on: angle-60
jeff, can we help you in some way ? This is breaking all non-tier1, and affects central and beta. The fix is simple, there are various options, but can we at least move forward to get back to a green state ?
Flags: needinfo?(jgilbert)
Comment on attachment 8955832 [details]
Bug 1442791 - Unbreak ANGLE build on Tier3 after bug 1440849.

https://reviewboard.mozilla.org/r/224860/#review235292

::: gfx/angle/checkout/src/common/system_utils_linux.cpp:33
(Diff revision 1)
>      char path[4096];
>  
> +#if defined(__linux__)
>      ssize_t result = readlink("/proc/self/exe", path, sizeof(path) - 1);
> +#elif defined(__sun)
> +    ssize_t result = readlink("/proc/self/path/a.out", path, sizeof(path) - 1);

This code isn't called from Gecko. Don't bother trying to make it portable.

::: gfx/angle/targets/angle_common/moz.build:125
(Diff revision 1)
>  ]
> -if CONFIG['OS_ARCH'] == 'Darwin':
> +if CONFIG['OS_ARCH'] in ('Darwin',):
>      SOURCES += [
>          '../../checkout/src/common/system_utils_mac.cpp',
>      ]
> -if CONFIG['OS_ARCH'] == 'Linux':
> +if CONFIG['OS_ARCH'] in ('Linux', 'SunOS', 'Darwin', 'DragonFly', 'FreeBSD', 'NetBSD', 'OpenBSD'):

This is awful. Is there not a more concise way to do this?

::: gfx/angle/update-angle.py:432
(Diff revision 1)
>              continue
>          elif e in ['cpp', 'cc']:
>              if b.endswith('_win'):
> -                sources_by_os_arch.setdefault('WINNT', []).append(x)
> +                sources_by_os_arch.setdefault("('WINNT',)", []).append(x)
>              elif b.endswith('_linux'):
> -                sources_by_os_arch.setdefault('Linux', []).append(x)
> +                sources_by_os_arch.setdefault("('Linux', 'SunOS', 'Darwin', 'DragonFly', 'FreeBSD', 'NetBSD', 'OpenBSD')", []).append(x)

Use actual tuples not a string.
Comment on attachment 8955833 [details]
Bug 1442791 - Try avoiding /proc on BSDs in GetExecutablePathImpl().

https://reviewboard.mozilla.org/r/224862/#review235304

::: gfx/angle/checkout/src/common/system_utils_linux.cpp:29
(Diff revision 1)
>  {
>  
>  namespace
>  {
>  
> +#if defined(KERN_PROC_PATHNAME)

Make a trivial stub, not a functional one. We don't call this code anyway. It merely needs to compile.
(In reply to Jan Beich from comment #5)
> A few issues remain:
> - update-angle.py is untested, requires Windows or I couldn't figure out how
> to use it (README_MOZILLA is missing)
> - _linux.cpp is shared with BSDs to avoid copypasta (both are ELF platforms)
>   and support /proc/self/exe but under /emul/linux (NetBSD) or /compat/linux
> (FreeBSD)
> - OpenBSD dropped /proc in 5.7 and never supported KERN_PROC_PATHNAME
> - Upstreaming is complicated because of Google Account walled garden; I
> don't care
>   about credit, so feel free to forward the changes as your own

It requires windows. This is not optional for the time being. I can handle upstreaming, but hopefully we can minimize what we need to upstream.
Flags: needinfo?(jgilbert)
Comment on attachment 8955832 [details]
Bug 1442791 - Unbreak ANGLE build on Tier3 after bug 1440849.

https://reviewboard.mozilla.org/r/224860/#review235292

> This is awful. Is there not a more concise way to do this?

> Is there not a more concise way to do this?

"Unix but not Apple" idiom from CMake world. See attachment 8955834 [details] [diff] [review] for an example.
Comment on attachment 8955833 [details]
Bug 1442791 - Try avoiding /proc on BSDs in GetExecutablePathImpl().

https://reviewboard.mozilla.org/r/224862/#review235304

> Make a trivial stub, not a functional one. We don't call this code anyway. It merely needs to compile.

Why fix what builds fine as is?
(In reply to Jeff Gilbert [:jgilbert] from comment #17)
> (In reply to Jan Beich from comment #5)
> > A few issues remain:
> > - update-angle.py is untested, requires Windows or I couldn't figure out how
>
> It requires windows. This is not optional for the time being.

I'm not familar with Windows, nor have a license.

> I can handle upstreaming, but hopefully we can minimize what we need to upstream.

If attachment 8955834 [details] [diff] [review] is the desired end result then there's nothing to upstream. It's all Gecko shenanigans in a NPOTB file.
Attachment #8955833 - Attachment is obsolete: true
Attachment #8955833 - Flags: review?(jgilbert)
Attachment #8955834 - Attachment is obsolete: true
I've uploaded something in-between. Implementing `else` logic is harder but the number of Tier3 platforms still alive in Gecko after aggressive oxidation is small.
Comment on attachment 8961174 [details]
Bug 1442791 - Build _linux files on BSDs in ANGLE. -

How does this look?
Attachment #8961174 - Flags: feedback?(jbeich)
Comment on attachment 8955832 [details]
Bug 1442791 - Unbreak ANGLE build on Tier3 after bug 1440849.

https://reviewboard.mozilla.org/r/224860/#review235654

Let's try the not-mac-not-win approach.
Attachment #8955832 - Flags: review?(jgilbert)
Comment on attachment 8955832 [details]
Bug 1442791 - Unbreak ANGLE build on Tier3 after bug 1440849.

https://reviewboard.mozilla.org/r/224860/#review235656
Comment on attachment 8961174 [details]
Bug 1442791 - Build _linux files on BSDs in ANGLE. -

https://reviewboard.mozilla.org/r/229942/#review235664
Attachment #8961174 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8961174 [details]
Bug 1442791 - Build _linux files on BSDs in ANGLE. -

I confirm, it builds fine on FreeBSD with the patch applied.
Attachment #8961174 - Flags: feedback?(jbeich) → feedback+
Attachment #8955832 - Attachment is obsolete: true
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e3aeeea7fc3
Build _linux files on BSDs in ANGLE. - r=jrmuizel
Can you request backport to Beta aka Firefox 60 (ESR) ?
Flags: needinfo?(jgilbert)
https://hg.mozilla.org/mozilla-central/rev/3e3aeeea7fc3
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8961174 [details]
Bug 1442791 - Build _linux files on BSDs in ANGLE. -

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1440849
[User impact if declined]: Can't build on some tier 2/3 configs
[Is this code covered by automated tests?]: build-only
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: build-only
[String changes made/needed]: none
Flags: needinfo?(jgilbert)
Attachment #8961174 - Flags: approval-mozilla-beta?
Comment on attachment 8961174 [details]
Bug 1442791 - Build _linux files on BSDs in ANGLE. -

bsd build fix, beta60+
Attachment #8961174 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: