Closed
Bug 927837
Opened 11 years ago
Closed 11 years ago
Don't use configure to generate substituted files / remove config.status --file/--header
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla28
People
(Reporter: ehsan.akhgari, Assigned: gps)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
I discovered this when trying to land bug 895047.
Assignee | ||
Comment 1•11 years ago
|
||
This is a subset of the problem "CONFIGURE_SUBST_FILES changes should trigger reconfigure."
The easy solution is to include CONFIGURE_SUBST_FILES in the backend.RecursiveMakeBackend.built dependency list. More involved (but better) is to add a build rule to process all of these at the top of a build. I /think/ that would work.
Assignee | ||
Comment 2•11 years ago
|
||
So we already track CONFIGURE_SUBST_FILES as "backend input files." Perhaps the backend.RecursiveMakeBackend.built dependency for js/src is off?
Assignee | ||
Comment 3•11 years ago
|
||
I was wrong in my assessment. js-confdefs.h is created via AC_OUTPUT in configure. So the general problem is "changes to AC_OUTPUT generated files are not detected."
This likely got regressed months ago.
Let me see what I can do.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•11 years ago
|
||
I believe this should do it.
Attachment #818669 -
Flags: review?(mh+mozilla)
Comment 5•11 years ago
|
||
Comment on attachment 818669 [details] [diff] [review]
Capture AC_OUTPUT and AC_HEADER_OUPUT dependencies to config.status
Review of attachment 818669 [details] [diff] [review]:
-----------------------------------------------------------------
While this seems sensible and landable, why not just move AC_OUTPUT and AC_HEADER_OUTPUT things out of configure.in in top-level moz.build as CONFIGURE_SUBST_FILES?
Attachment #818669 -
Flags: review?(mh+mozilla) → feedback+
Assignee | ||
Comment 6•11 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #5)
> While this seems sensible and landable, why not just move AC_OUTPUT and
> AC_HEADER_OUTPUT things out of configure.in in top-level moz.build as
> CONFIGURE_SUBST_FILES?
I can try that. But something in the back of my head is telling me we tried this once and it didn't work for some reason. It /should/ work though.
Assignee | ||
Comment 7•11 years ago
|
||
Assignee | ||
Comment 8•11 years ago
|
||
I killed files and headers as explicit lists in config.status, added
header support to moz.build, and moved everything over.
Attachment #819133 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #819131 -
Attachment is obsolete: true
Comment 9•11 years ago
|
||
Comment on attachment 819133 [details] [diff] [review]
Capture AC_HEADER_OUTPUT in moz.build
Review of attachment 819133 [details] [diff] [review]:
-----------------------------------------------------------------
::: build/ConfigStatus.py
@@ -99,5 @@
>
> if options.recheck:
> # Execute configure from the top object directory
> - if not os.path.isabs(topsrcdir):
> - topsrcdir = relpath(topsrcdir, topobjdir)
I'd rather not touch this here, it's unrelated. Please file a separate bug if you want to remove this.
@@ +125,4 @@
> for file in files:
> env.create_config_file(file)
> for header in headers:
> env.create_config_header(header)
Maybe it's time to unsupport --file and --header ; --header is unused, and --file is only used for Makefiles, and that's wrong because that doesn't take care of the corresponding backend.mk.
::: build/autoconf/config.status.m4
@@ -146,5 @@
> ->>>)dnl
> -
> -cat >> $CONFIG_STATUS <<\EOF
> -]
> -
Can you modify AC_OUTPUT to barf when given arguments, and add a AC_CONFIG_HEADER that barfs when used?
::: python/mozbuild/mozbuild/frontend/data.py
@@ -108,3 @@
>
> - The output_path attribute defines the relative path from topsrcdir of the
> - output file to generate.
Might as well keep the comment about output_path.
::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +363,5 @@
> + """Header files generated from configure/config.status.
> +
> + This is a substitute for ``AC_CONFIG_HEADER`` in autoconf. This is very
> + similar to ``CONFIGURE_SUBST_FILES`` except the generation logic takes
> + into account the values of ``AC_DEFINE`` in addition to ``AC_SUBST``.
AC_SUBST isn't taken into account for header files.
Attachment #819133 -
Flags: review?(mh+mozilla) → feedback+
Assignee | ||
Comment 10•11 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #9)
> ::: build/ConfigStatus.py
> @@ -99,5 @@
> >
> > if options.recheck:
> > # Execute configure from the top object directory
> > - if not os.path.isabs(topsrcdir):
> > - topsrcdir = relpath(topsrcdir, topobjdir)
>
> I'd rather not touch this here, it's unrelated. Please file a separate bug
> if you want to remove this.
Ah, forgot to comment on this in patch notes. My automatic static analysis on commit hook told me that 'relpath' is undefined. Sure enough, it isn't. So this branch is broken today. But I can file a new bug easily enough.
> @@ +125,4 @@
> > for file in files:
> > env.create_config_file(file)
> > for header in headers:
> > env.create_config_header(header)
>
> Maybe it's time to unsupport --file and --header ; --header is unused, and
> --file is only used for Makefiles, and that's wrong because that doesn't
> take care of the corresponding backend.mk.
Gladly.
Assignee | ||
Updated•11 years ago
|
Attachment #818669 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #819133 -
Attachment is obsolete: true
Assignee | ||
Comment 11•11 years ago
|
||
I killed --header and --file support. This meant removing a number of
references to config.status in rules.mk.
I'll be honest, the removal of NO_MAKEFILE_RULE and NO_SUBMAKEFILES_RULE
kinda scares me. We'll be fine as long as we don't have any "standalone"
make files in the tree that aren't accounted for in the moz.build
domain. Do we? I'm, uh, not sure. Probably. I wouldn't be surprised if
an installer or packager Makefile.in wasn't referenced in a moz.build
file.
I'm not sure why the AC_OUTPUT test for $# is reporting 1 instead of 0.
My m4 skills aren't that great :/
https://tbpl.mozilla.org/?tree=Try&rev=c5646e051d9b
Attachment #821847 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•11 years ago
|
Summary: Touching js/src/js-confdefs.h.in requires a clobber → Don't use configure to generate substituted files / remove config.status --file/--header
Comment 12•11 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #11)
> I'll be honest, the removal of NO_MAKEFILE_RULE and NO_SUBMAKEFILES_RULE
> kinda scares me. We'll be fine as long as we don't have any "standalone"
> make files in the tree that aren't accounted for in the moz.build
> domain. Do we? I'm, uh, not sure. Probably. I wouldn't be surprised if
> an installer or packager Makefile.in wasn't referenced in a moz.build
> file.
Or worse... l10n. I also realized I'm using config.status --file for iceweasel packaging, so I've actually shot myself in the foot with comment 9 :-/
So maybe it's not the right time to unsupport those... I'm not sure.
Comment 13•11 years ago
|
||
Comment on attachment 821847 [details] [diff] [review]
Capture AC_HEADER_OUTPUT in moz.build
Review of attachment 821847 [details] [diff] [review]:
-----------------------------------------------------------------
Actually, I /think/ this is safe. I'd land that after the merge, though.
::: build/ConfigStatus.py
@@ +49,5 @@
> '''
>
> if 'CONFIG_FILES' in os.environ:
> + raise Exception('Using the CONFIG_FILES environment variable is not '
> + 'supported. Use --file instead.')
If you remove --file, You need to remove messages about it :)
@@ +54,3 @@
> if 'CONFIG_HEADERS' in os.environ:
> + raise Exception('Using the CONFIG_HEADERS environment variable is not '
> + 'supported. Use --header instead.')
Likewise for --header.
Attachment #821847 -
Flags: review?(mh+mozilla) → review+
Comment 14•11 years ago
|
||
Comment on attachment 821847 [details] [diff] [review]
Capture AC_HEADER_OUTPUT in moz.build
Review of attachment 821847 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/build/autoconf/config.status.m4
@@ +154,5 @@
> exit 1
> fi
> ])
>
> +define([AC_OUTPUT], [ifelse($#, 1, [_MOZ_AC_OUTPUT()],
Ah, about this. There is always an argument, so $# is at least 1. The sensible way to do the check we want is to do ifelse($#_$1, 1_, ...)
@@ +155,5 @@
> fi
> ])
>
> +define([AC_OUTPUT], [ifelse($#, 1, [_MOZ_AC_OUTPUT()],
> +[AC_MSG_ERROR([Use CONFIGURE_SUBST_FILES in moz.build files to create substituted files.])]
AC_MSG_ERROR is going to error out when running configure, not when running autoconf. I'm not sure how to error out at autoconf time. I'll try to dig this up.
Comment 15•11 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #14)
> I'm not sure how to error out at autoconf time. I'll try to dig
> this up.
define([m4_fatal],[
errprint([$1
])
m4exit(1)
])
(Hoping it works on mac, too, not sure what m4 OSX has)
Comment 16•11 years ago
|
||
So, this would likely break static analysis builds: if i remove the test to add build/clang-plugin to the base tier, build/clang-plugin/Makefile is not created.
Comment 17•11 years ago
|
||
But that's probably the only bad case.
Comment 18•11 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #16)
> So, this would likely break static analysis builds: if i remove the test to
> add build/clang-plugin to the base tier, build/clang-plugin/Makefile is not
> created.
It's actually not a problem. That file is created by a subconfigure.
Assignee | ||
Comment 19•11 years ago
|
||
Applied feedback. Will land after merge.
https://tbpl.mozilla.org/?tree=Try&rev=db03044ceb7c
Assignee | ||
Comment 20•11 years ago
|
||
Flags: in-testsuite-
Target Milestone: --- → mozilla28
Comment 21•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
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
•