Closed
Bug 785871
Opened 12 years ago
Closed 12 years ago
Make config.status importable
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla18
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(2 files, 4 obsolete files)
12.34 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
4.08 KB,
patch
|
ted
:
review+
gps
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•12 years ago
|
||
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 2•12 years ago
|
||
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+
Assignee | ||
Comment 3•12 years ago
|
||
Is it what you were thinking about?
Attachment #655651 -
Flags: review?(gps)
Assignee | ||
Updated•12 years ago
|
Attachment #655579 -
Attachment is obsolete: true
Assignee | ||
Comment 4•12 years ago
|
||
Attachment #655666 -
Flags: review?(gps)
Assignee | ||
Updated•12 years ago
|
Attachment #655651 -
Attachment is obsolete: true
Attachment #655651 -
Flags: review?(gps)
Comment 5•12 years ago
|
||
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+
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #655966 -
Flags: review?(ted.mielczarek)
Attachment #655966 -
Flags: review?(gps)
Comment 7•12 years ago
|
||
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)
Assignee | ||
Comment 8•12 years ago
|
||
(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
Comment 9•12 years ago
|
||
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+
Assignee | ||
Comment 10•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f3daa00593c
Whiteboard: [leave open]
Assignee | ||
Comment 12•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Attachment #655966 -
Attachment is obsolete: true
Attachment #655966 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 13•12 years ago
|
||
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)
Assignee | ||
Comment 14•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Attachment #656930 -
Attachment is obsolete: true
Comment 15•12 years ago
|
||
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+
Assignee | ||
Comment 16•12 years ago
|
||
(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 17•12 years ago
|
||
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+
Assignee | ||
Comment 18•12 years ago
|
||
(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?
Comment 19•12 years ago
|
||
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.
Assignee | ||
Comment 20•12 years ago
|
||
How about build_config or buildconfig?
Comment 21•12 years ago
|
||
sold.
Assignee | ||
Comment 22•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d77b2fff1a1
Whiteboard: [leave open]
Comment 23•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2d77b2fff1a1
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•