Closed Bug 785871 Opened 8 years ago Closed 8 years ago

Make config.status importable

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla18

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(2 files, 4 obsolete files)

No description provided.
Attached patch Make config.status importable (obsolete) — Splinter Review
This allows to import the config.status script with:
  import imp
  config = imp.load_module('config', open('/path/to/config.status'), '/path/to/config.status', ('', 'r', imp.PY_SOURCE))
Attachment #655579 - Flags: review?(khuey)
Comment on attachment 655579 [details] [diff] [review]
Make config.status importable

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

This patch makes me so happy. What we effectively have now is reusable API to the build system \o/

I granted r+ but I would /really/ like the 2nd comment addressed before commit.

::: Makefile.in
@@ +68,5 @@
>  
>  include $(topsrcdir)/config/config.mk
>  
>  GARBAGE_DIRS += dist _javagen _profile _tests staticlib
> +DIST_GARBAGE = config.cache config.log config.status* config-defs.h \

The wildcard makes me sad. But, I think the alternative is sadder.

::: build/autoconf/config.status.m4
@@ +157,4 @@
>  
>  dnl Do the actual work
> +if __name__ == '__main__':
> +    args = dict([(name, value) for name, value in globals().items() if not name in ['name', 'value', 'os'] and not name.startswith('__')])

This is too much magic for my liking.

What about defining __all__ and using that?
Attachment #655579 - Flags: review?(khuey) → review+
Attached patch Make config.status importable (obsolete) — Splinter Review
Is it what you were thinking about?
Attachment #655651 - Flags: review?(gps)
Attachment #655579 - Attachment is obsolete: true
Attachment #655666 - Flags: review?(gps)
Attachment #655651 - Attachment is obsolete: true
Attachment #655651 - Flags: review?(gps)
Comment on attachment 655666 [details] [diff] [review]
Make config.status importable

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

If Try passes, land it.
Attachment #655666 - Flags: review?(gps) → review+
Attachment #655966 - Flags: review?(ted.mielczarek)
Attachment #655966 - Flags: review?(gps)
Comment on attachment 655966 [details] [diff] [review]
Make importing config.status easier

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

I think mozconfig.py is just a giant hack.

Instead of jumping through hoops to import config.status and re-export its symbols, I would just write out the static config bits to its own file at autoconf time. config.status (or whatever) can import this module directly (possibly provide virtualenv magic to ensure the path is registered, etc). In other words, separate the static data from the code. This will eliminate all the import hacking which, quite frankly, scares me.

This means that config.status will likely be static and mozconfig.py will be dynamic. I realize this has implications for the build system, as we use config.status as a dependency in a lot of places. Perhaps it's time for us to reconsider how configure and the build backend play together. I understand this is a controversial position. We disagreed about it over IRC. I'd like Ted to weigh in here. If Ted thinks I'm on crack, I'll happily ignore complaints about hackiness and will continue the review.

::: build/mozconfig.py
@@ +1,1 @@
> +import imp, os, sys

MPL
Attachment #655966 - Flags: review?(gps)
(In reply to Gregory Szorc [:gps] from comment #7)
> Comment on attachment 655966 [details] [diff] [review]
> Make importing config.status easier
> 
> Review of attachment 655966 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think mozconfig.py is just a giant hack.
> 
> Instead of jumping through hoops to import config.status and re-export its
> symbols, I would just write out the static config bits to its own file at
> autoconf time. config.status (or whatever) can import this module directly
> (possibly provide virtualenv magic to ensure the path is registered, etc).
> In other words, separate the static data from the code. This will eliminate
> all the import hacking which, quite frankly, scares me.

The immediate problem is that this approach doesn't work for js/src, and this "hack" simplifies some of my modules in bug 780561.
The one problem with the current implementation of the hack is that it doesn't work properly if used in comm-central. It should do up from os.curdir instead of __file__ for that to work.
Blocks: new-packager
Blocks: 585012
Comment on attachment 655966 [details] [diff] [review]
Make importing config.status easier

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

Reluctant r+ because of hacks. But, it's a step in the right direction, so I approve.

::: build/mozconfig.py
@@ +1,5 @@
> +import imp, os, sys
> +
> +path = os.path.dirname(__file__)
> +while not os.path.exists(os.path.join(path, 'config.status')):
> +    parent = os.path.normpath(os.path.join(path, '..'))

Nit: use os.pardir instead of '..'. Or, do we care?

@@ +3,5 @@
> +path = os.path.dirname(__file__)
> +while not os.path.exists(os.path.join(path, 'config.status')):
> +    parent = os.path.normpath(os.path.join(path, '..'))
> +    if parent == path:
> +        raise Exception, '''Can't find config.status'''

Nit: Just use double quotes? I don't think inline triple quoting is usually used outside of docstrings unless you are doing multiple line.
Nit: Exception("foo"). This works in Python 2.5. It's the exception handling that requires the comma crap in 2.5.

::: build/virtualenv/populate_virtualenv.py
@@ +16,5 @@
>      """Populate the virtualenv from the contents of a manifest.
>  
>      The manifest file consists of colon-delimited fields. The first field
>      specifies the action. The remaining fields are arguments to that action.
>      The following actions are supported:

Please add documentation here.
Attachment #655966 - Flags: review+
This adds AC_SUBST value overrides through environment variables, a bit like what make would do.
Attachment #656930 - Flags: review?(ted.mielczarek)
Attachment #656930 - Flags: review?(gps)
Attachment #655966 - Attachment is obsolete: true
Attachment #655966 - Flags: review?(ted.mielczarek)
Comment on attachment 656930 [details] [diff] [review]
Make importing config.status easier

Gah, forgot to update with previous review.
Attachment #656930 - Flags: review?(ted.mielczarek)
Attachment #656930 - Flags: review?(gps)
This adds AC_SUBST value overrides through environment variables, a bit like what make would do, and addresses gps' comments.
Attachment #656935 - Flags: review?(ted.mielczarek)
Attachment #656935 - Flags: review?(gps)
Attachment #656930 - Attachment is obsolete: true
Comment on attachment 656935 [details] [diff] [review]
Make importing config.status easier

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

Just a handful of nits. I trust you to address them before landing.

::: build/mozconfig.py
@@ +1,1 @@
> +import imp, os, sys

Need MPL license.
Nit: 1 line per import per recommended Python style.

@@ +13,5 @@
> +for var in config.__all__:
> +    value = getattr(config, var)
> +    if isinstance(value, list) and isinstance(value[0], tuple):
> +        value = dict(value)
> +    setattr(sys.modules[__name__], var, value)

I think the following will work:

  setattr(globals(), var, value)

since globals() evaluates to the module from within a module. This feels slightly less hacky to me than going through sys.modules.

@@ +17,5 @@
> +    setattr(sys.modules[__name__], var, value)
> +
> +for var in os.environ:
> +    if var in substs:
> +        substs[var] = os.environ[var]

In our ideal world, I'd say that this shouldn't be needed: everything would be determined at configure time and the environment at run-time wouldn't matter. This guarantees idempotent behavior of builds, etc. But, we are so far away from that today (make is the biggest offender since variables can magically come from the environment) that this doesn't worry me too much.

::: build/virtualenv/populate_virtualenv.py
@@ +43,5 @@
>              assert len(package) >= 2
>  
>              call_setup(os.path.join(top_source_directory, package[1]),
>                  package[2:])
> +        elif package[0] == "copy":

assert len(package) == 2 ?

@@ +46,5 @@
>                  package[2:])
> +        elif package[0] == "copy":
> +            shutil.copy(os.path.join(top_source_directory, package[1]),
> +                        os.path.join(distutils.sysconfig.get_python_lib(), os.path.basename(package[1])))
> +        elif package[0].endswith('.pth'):

Bonus points will be awarded for pulling distutils.sysconfig.get_python_lib() into a reused variable.
Attachment #656935 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #15)
> In our ideal world, I'd say that this shouldn't be needed: everything would
> be determined at configure time and the environment at run-time wouldn't
> matter. This guarantees idempotent behavior of builds, etc. But, we are so
> far away from that today (make is the biggest offender since variables can
> magically come from the environment) that this doesn't worry me too much.

Even in an ideal world, everything is not so static. It's useful to be able to do one-off change of a variable without going through a full reconfigure. Think make package PKG_SKIP_STRIP=1 and other such "tricks".
Comment on attachment 656935 [details] [diff] [review]
Make importing config.status easier

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

::: build/mozconfig.py
@@ +1,1 @@
> +import imp, os, sys

Can we name this something other than "mozconfig"? That has a lot of connotation to it in the Mozilla world.
Attachment #656935 - Flags: review?(ted.mielczarek) → review+
(In reply to Ted Mielczarek [:ted] from comment #17)
> Can we name this something other than "mozconfig"? That has a lot of
> connotation to it in the Mozilla world.

Would you have any suggestions?
mozilla_tree_config
tree_config
configure_output

I'm not too worried about the name because only low-level build system code will consume it. If we get worried about name conflicts, we can always hook up imp or execfile.
How about build_config or buildconfig?
https://hg.mozilla.org/mozilla-central/rev/2d77b2fff1a1
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Blocks: 825872
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.