1305958 - Allow custom GENERATED_FILES in SYMBOLS_FILE

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

NSS has its own special format for symbol export files--they're processed with some awful sed goop on Linux/OS X and just used directly on Windows. To support this I need to be able to use `SYMBOLS_FILE` but specify my own custom script via `GENERATED_FILES`.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: bug 1305958 - Allow custom GENERATED_FILES in SYMBOLS_FILE.

This patch allows specifying an objdir path in `SYMBOLS_FILE`, with the
requirement that the file is also listed in `GENERATED_FILES`. This is used
for handling NSS' .def files with their special processing.

I added tests for the existing `SYMBOLS_FILE` case as well as the new case
and a test for the error if the file is not listed in `GENERATED_FILES`.

Review commit: https://reviewboard.mozilla.org/r/81636/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/81636/

Comment 2

[(not currently active) Ted Mielczarek]

I don't know what's up with those busted windows builds on try. I rebased locally and built and it built OK, so I'll push that to try and see how it goes.

Comment 3

[(not currently active) Ted Mielczarek]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fb9e69a4a59

Comment 4

[(not currently active) Ted Mielczarek]

It's still very busted on Try, so that's exciting.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8795668 [details]
bug 1305958 - Allow custom GENERATED_FILES in SYMBOLS_FILE.

https://reviewboard.mozilla.org/r/81636/#review81556

::: python/mozbuild/mozbuild/frontend/emitter.py:682
(Diff revision 1)
> -                if not os.path.exists(symbols_file.full_path):
> +                    if not os.path.exists(symbols_file.full_path):
> -                    raise SandboxValidationError(
> +                        raise SandboxValidationError(
> -                        'Path specified in SYMBOLS_FILE does not exist: %s '
> +                            'Path specified in SYMBOLS_FILE does not exist: %s '
> -                        '(resolved to %s)' % (symbols_file,
> +                            '(resolved to %s)' % (symbols_file,
> -                        symbols_file.full_path), context)
> +                            symbols_file.full_path), context)
> -                shared_args['symbols_file'] = True
> +                    shared_args['symbols_file'] = symbols_file.target_basename + '.def'

You shouldn't be adding a .def here. This leads to weirdness like .def.def files in the objdir.
I'd rather keep the current name deriving logic in data.SharedLibrary, and make its constructor take a value that is either False (no symbol file), True (symbol file with the default name), or a file name, for the case you're adding.

::: python/mozbuild/mozbuild/frontend/emitter.py:684
(Diff revision 1)
> -                        'Path specified in SYMBOLS_FILE does not exist: %s '
> +                            'Path specified in SYMBOLS_FILE does not exist: %s '
> -                        '(resolved to %s)' % (symbols_file,
> +                            '(resolved to %s)' % (symbols_file,
> -                        symbols_file.full_path), context)
> +                            symbols_file.full_path), context)
> -                shared_args['symbols_file'] = True
> +                    shared_args['symbols_file'] = symbols_file.target_basename + '.def'
> +                else:
> +                    if symbols_file.target_basename not in generated_files:

You also need to check that the file is in the current objdir (e.g. not !/foo or !../foo) because those won't work.

Comment 6

[(not currently active) Ted Mielczarek]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a69589c1233

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 8795668 [details]
bug 1305958 - Allow custom GENERATED_FILES in SYMBOLS_FILE.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81636/diff/1-2/

Comment 8

[(not currently active) Ted Mielczarek]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #6)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a69589c1233

This is pretty green, except for that one Win64 build that seems to have failed in some unrelated sccache weirdness.

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8795668 [details]
bug 1305958 - Allow custom GENERATED_FILES in SYMBOLS_FILE.

https://reviewboard.mozilla.org/r/81636/#review82736

::: python/mozbuild/mozbuild/frontend/emitter.py:661
(Diff revision 2)
> -                        'Path specified in SYMBOLS_FILE does not exist: %s '
> +                            'Path specified in SYMBOLS_FILE does not exist: %s '
> -                        '(resolved to %s)' % (symbols_file,
> +                            '(resolved to %s)' % (symbols_file,
> -                        symbols_file.full_path), context)
> +                            symbols_file.full_path), context)
> -                shared_args['symbols_file'] = True
> +                    shared_args['symbols_file'] = True
> +                else:
> +                    if symbols_file.target_basename not in generated_files:

Looking at the code wrt my previous comment around here reveals something interesting: generated_files is a superset of GENERATED_FILES.

Comment 10

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e5ed19b4979e21554ef44835bb98db06fc96731
bug 1305958 - Allow custom GENERATED_FILES in SYMBOLS_FILE. r=glandium

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1e5ed19b4979