Closed Bug 927837 Opened 6 years ago Closed 6 years ago

Don't use configure to generate substituted files / remove config.status --file/--header

Categories

(Firefox Build System :: General, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla28

People

(Reporter: ehsan, Assigned: gps)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

I discovered this when trying to land bug 895047.
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.
So we already track CONFIGURE_SUBST_FILES as "backend input files." Perhaps the backend.RecursiveMakeBackend.built dependency for js/src is off?
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
I believe this should do it.
Attachment #818669 - Flags: review?(mh+mozilla)
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+
(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.
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)
Attachment #819131 - Attachment is obsolete: true
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+
(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.
Attachment #818669 - Attachment is obsolete: true
Attachment #819133 - Attachment is obsolete: true
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)
Summary: Touching js/src/js-confdefs.h.in requires a clobber → Don't use configure to generate substituted files / remove config.status --file/--header
(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 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 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.
(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)
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.
But that's probably the only bad case.
(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.
Applied feedback. Will land after merge.

https://tbpl.mozilla.org/?tree=Try&rev=db03044ceb7c
https://hg.mozilla.org/integration/mozilla-inbound/rev/58ca27d61309
Flags: in-testsuite-
Target Milestone: --- → mozilla28
https://hg.mozilla.org/mozilla-central/rev/58ca27d61309
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Blocks: clobber
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.