Closed Bug 1060210 Opened 7 years ago Closed 7 years ago

Automatically import CppEclipse project into the workspace

Categories

(Firefox Build System :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla35

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — 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 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+
Attached patch patch v2Splinter Review
Assignee: nobody → bgirard
Attachment #8481082 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8483684 - Flags: review?(gps)
Attachment #8483684 - Attachment is patch: true
(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.
Attachment #8483684 - Flags: review?(gps) → review+
https://hg.mozilla.org/mozilla-central/rev/decfb59e691c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1408810
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.