Closed
Bug 1060210
Opened 10 years ago
Closed 10 years ago
Automatically import CppEclipse project into the workspace
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla35
People
(Reporter: BenWa, Assigned: BenWa)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
Before you had to import and index the project in one step. The former takes 10 seconds, the latter can take up to 20 minutes.
Here's a hack that's disables the indexer, imports the project, and restores the indexer.
Attachment #8481082 -
Flags: review?(gps)
Comment 1•10 years ago
|
||
Comment on attachment 8481082 [details] [diff] [review]
patch
Review of attachment 8481082 [details] [diff] [review]:
-----------------------------------------------------------------
::: python/mozbuild/mozbuild/backend/cpp_eclipse.py
@@ +38,5 @@
> self._cppflags = self.environment.substs.get('CPPFLAGS', '')
>
> def detailed(summary):
> + return ('Generated Cpp Eclipse workspace in "%s".\n' + \
> + 'If missing, omport the project using File > Import > General > Existing Project into workspace\n' + \
"import"
@@ +111,5 @@
> + # Now import the project into the workspace
> + self._import_project()
> +
> + def _import_project(self):
> + # If the workspace already exists then don't import the project against because
"again"
@@ +117,5 @@
> + if self._overwriting_workspace:
> + return
> +
> + # We disable the indexer otherwise we're forced to index
> + # the whole codebase when importing the porject. This goes from 20 mins to 10 seconds.
"project"
Also, I think it's clearer if you say "indexing the project can take 20 minutes". The "this goes from" comment is more appropriate for the commit message, which is blank, sadly.
@@ +121,5 @@
> + # the whole codebase when importing the porject. This goes from 20 mins to 10 seconds.
> + self._write_noindex()
> +
> + from mozprocess import ProcessHandlerMixin
> + import functools
Please import these at file level.
@@ +125,5 @@
> + import functools
> + def echo_line(s):
> + print s
> + process = ProcessHandlerMixin(
> + ["eclipse", "-application", "-nosplash",
What if 'eclipse' isn't in PATH?
@@ +131,5 @@
> + "-data", self._workspace_dir, "-importAll", self._project_dir],
> + processOutputLine=echo_line,
> + universal_newlines=True)
> + process.run()
> + status = process.wait()
We could probably just use subprocess.check_call() here.
@@ +133,5 @@
> + universal_newlines=True)
> + process.run()
> + status = process.wait()
> +
> + self._remove_noindex()
You may want to put this _write_noindex() and _remove_noindex() inside a try..finally to help ensure things don't get in an inconsistent state.
Attachment #8481082 -
Flags: review?(gps) → feedback+
Assignee | ||
Comment 2•10 years ago
|
||
Assignee: nobody → bgirard
Attachment #8481082 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8483684 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8483684 -
Attachment is patch: true
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Gregory Szorc [:gps] (away Sep 10 through 27) from comment #1)
> @@ +125,5 @@
> > + import functools
> > + def echo_line(s):
> > + print s
> > + process = ProcessHandlerMixin(
> > + ["eclipse", "-application", "-nosplash",
>
> What if 'eclipse' isn't in PATH?
This effectively makes it mandatory to have it in your path. I think its the right thing because it will allow us to call eclipse to perform task that are difficult to do by manually manipulating the files.
Eventually we will want to make these errors more friendly. My plan is to later introduce a ./mach eclipse target which will be a friendly front end and likely check that the pre-req, like eclipse, can be found.
Updated•10 years ago
|
Attachment #8483684 -
Flags: review?(gps) → review+
Assignee | ||
Comment 4•10 years ago
|
||
Comment 5•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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
•