1060210 - Automatically import CppEclipse project into the workspace

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: patch

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.

Comment 1

[Gregory Szorc [: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.

Comment 2

[Benoit Girard (:BenWa)]

Attachment: patch v2

Comment 3

[Benoit Girard (:BenWa)]

(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.

Comment 4

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/decfb59e691c

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/decfb59e691c