1062726 - Add an 'eclipse' mach target

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[Benoit Girard (:BenWa)]

Right now the eclipse project generation is only available via a mach build-backend. This bug aims to provide a friendly mach target.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 2

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8483986 [details] [diff] [review]
patch

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

Fly-by.  If we do land something like this, I'd like to see it disabled for Android builders.  I'll follow-up to make it do the right thing for Android Eclipse users.

::: python/mozbuild/mozbuild/backend/mach_commands.py
@@ +17,5 @@
> +    CommandProvider,
> +    Command,
> +)
> +
> +# Form: http://stackoverflow.com/questions/377017/test-if-executable-exists-in-python

nit: From.

@@ +52,5 @@
> +        # this fairly foolproof.
> +        res = self._mach_context.commands.dispatch('build', self._mach_context)
> +
> +        # Generate or refresh the eclipse workspace
> +        res = self._mach_context.commands.dispatch('build-backend', self._mach_context, **{'backend': 'CppEclipse'})

At this point, we have three layers of indirection: |mach eclipse| -> |mach build-backend| -> config_status.py.  It's already hard to pass parameters down that chain.  I suggest that we revisit the |build-backend -b| decision and instead surface |build-backend -b| as top-level mach commands themselves.  That will allow for richer backend commands.  That might mean making the traversal in config_status.py easier to re-use.

Comment 3

[Benoit Girard (:BenWa)]

(In reply to Nick Alexander :nalexander from comment #2)
> Comment on attachment 8483986 [details] [diff] [review]
> patch
> 
> Review of attachment 8483986 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Fly-by.  If we do land something like this, I'd like to see it disabled for
> Android builders.  I'll follow-up to make it do the right thing for Android
> Eclipse users.

What do you suggest the right thing be here? This patch here is basically saying that 'mach eclipse' is defaulting to CppEclipse. This is unfortunate since we have a backend for the Android Java stuff. But I think it's better to be consistent on all platform and generate the same thing everywhere.

IMO the right thing to do is to generate an Android Java project and import that into the same workspace. 

> 
> ::: python/mozbuild/mozbuild/backend/mach_commands.py
> @@ +17,5 @@
> > +    CommandProvider,
> > +    Command,
> > +)
> > +
> > +# Form: http://stackoverflow.com/questions/377017/test-if-executable-exists-in-python
> 
> nit: From.
> 
> @@ +52,5 @@
> > +        # this fairly foolproof.
> > +        res = self._mach_context.commands.dispatch('build', self._mach_context)
> > +
> > +        # Generate or refresh the eclipse workspace
> > +        res = self._mach_context.commands.dispatch('build-backend', self._mach_context, **{'backend': 'CppEclipse'})
> 
> At this point, we have three layers of indirection: |mach eclipse| -> |mach
> build-backend| -> config_status.py.  It's already hard to pass parameters
> down that chain.  I suggest that we revisit the |build-backend -b| decision
> and instead surface |build-backend -b| as top-level mach commands
> themselves.  That will allow for richer backend commands.  That might mean
> making the traversal in config_status.py easier to re-use.

I don't think think there's anything wrong with the 3 levels since they each have their own clearly defined purpose. I don't feel too strongly on this but we shouldn't gate this patch on it.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8483986 [details] [diff] [review]
patch

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

::: python/mozbuild/mozbuild/backend/mach_commands.py
@@ +18,5 @@
> +    Command,
> +)
> +
> +# Form: http://stackoverflow.com/questions/377017/test-if-executable-exists-in-python
> +def which(program):

import which
try:
    return which.which(program)
except which.WhichError:
    return None

@@ +42,5 @@
> +    @Command('eclipse', category='devenv',
> +        allow_all_args=True,
> +        description='Generate and launch an eclipse project.')
> +    @CommandArgument('args', nargs=argparse.REMAINDER)
> +    def eclipse(self, args):

I can go both ways on |mach eclipse|. On one hand, it's short and concise and does something useful (I think). On the other, we'd have to establish |mach visual-studio|, |mach xcode|, etc. I'm not a huge fan of that. That concern can be somewhat abated by setting a condition on this mach command to only work (and show up in the UI) if "eclipse" is found in the path. See http://dxr.mozilla.org/mozilla-central/source/build/valgrind/mach_commands.py#36 for conditions in action.

@@ +45,5 @@
> +    @CommandArgument('args', nargs=argparse.REMAINDER)
> +    def eclipse(self, args):
> +        if which('eclipse') == None:
> +            print('Eclipse CDT 8.4 or later must be installed in your PATH.\nDownload: http://www.eclipse.org/cdt/downloads.php')
> +            return

return 1 to signal non-success.

@@ +49,5 @@
> +            return
> +
> +        # Refresh the build. We might want to revisit this decision but it should make
> +        # this fairly foolproof.
> +        res = self._mach_context.commands.dispatch('build', self._mach_context)

Wait, we're doing a full build in order to generate and launch Eclipse projects?!

I know that may be a precondition of the CppEclipse project today. But it seems ridiculous to require a build to launch Eclipse.

@@ +52,5 @@
> +        # this fairly foolproof.
> +        res = self._mach_context.commands.dispatch('build', self._mach_context)
> +
> +        # Generate or refresh the eclipse workspace
> +        res = self._mach_context.commands.dispatch('build-backend', self._mach_context, **{'backend': 'CppEclipse'})

E_TOO_MUCH_INDIRECTION.

mach build-backend does very little. The indirection isn't useful. I'm fine with invoking config.status directly.

Also, there is ambiguity between CppEclipse and Android Eclipse. Yes, they should probably be merged into the same backend. But I don't like introducing confusion for the Fennec developers. Can we make |mach eclipse| pick the most appropriate one automagically?

@@ +58,5 @@
> +        workspace_dir = self.get_workspace_path()
> +        process = subprocess.check_call(['eclipse', '-data', workspace_dir])
> +
> +    def get_workspace_path(self):
> +        # This must match python/mozbuild/mozbuild/backend/cpp_eclipse.py

Then refactor the calculation to exist in a reusable function.

Comment 5

[Benoit Girard (:BenWa)]

(In reply to Gregory Szorc [:gps] (away Sep 10 through 27) from comment #4)
> @@ +42,5 @@
> > +    @Command('eclipse', category='devenv',
> > +        allow_all_args=True,
> > +        description='Generate and launch an eclipse project.')
> > +    @CommandArgument('args', nargs=argparse.REMAINDER)
> > +    def eclipse(self, args):
> 
> I can go both ways on |mach eclipse|. On one hand, it's short and concise
> and does something useful (I think). On the other, we'd have to establish
> |mach visual-studio|, |mach xcode|, etc. I'm not a huge fan of that. That
> concern can be somewhat abated by setting a condition on this mach command
> to only work (and show up in the UI) if "eclipse" is found in the path. See
> http://dxr.mozilla.org/mozilla-central/source/build/valgrind/mach_commands.
> py#36 for conditions in action.

This makes the command less discoverable.

> 
> @@ +45,5 @@
> > +    @CommandArgument('args', nargs=argparse.REMAINDER)
> > +    def eclipse(self, args):
> > +        if which('eclipse') == None:
> > +            print('Eclipse CDT 8.4 or later must be installed in your PATH.\nDownload: http://www.eclipse.org/cdt/downloads.php')
> > +            return
> 
> return 1 to signal non-success.
> 
> @@ +49,5 @@
> > +            return
> > +
> > +        # Refresh the build. We might want to revisit this decision but it should make
> > +        # this fairly foolproof.
> > +        res = self._mach_context.commands.dispatch('build', self._mach_context)
> 
> Wait, we're doing a full build in order to generate and launch Eclipse
> projects?!
> 
> I know that may be a precondition of the CppEclipse project today. But it
> seems ridiculous to require a build to launch Eclipse.

It's needed to generate the project. This is a proper dependency. Also it means that you can generate an eclipse project without having to know what the dependencies are (mental overhead). This also means that you can do: hg pull && ./mach eclipse. This will give me an update environment ready for development.

> 
> @@ +52,5 @@
> > +        # this fairly foolproof.
> > +        res = self._mach_context.commands.dispatch('build', self._mach_context)
> > +
> > +        # Generate or refresh the eclipse workspace
> > +        res = self._mach_context.commands.dispatch('build-backend', self._mach_context, **{'backend': 'CppEclipse'})
> 
> E_TOO_MUCH_INDIRECTION.
> 
> mach build-backend does very little. The indirection isn't useful. I'm fine
> with invoking config.status directly.
> 
> Also, there is ambiguity between CppEclipse and Android Eclipse. Yes, they
> should probably be merged into the same backend. But I don't like
> introducing confusion for the Fennec developers. Can we make |mach eclipse|
> pick the most appropriate one automagically?

How would it do the right thing automagically? If people are used to using eclipse for platform development why would they get something different because we happen to have some way to generate a front end project. What I'm doing here isn't regressing anything that's provided and is consistent across all platform. In the future the workspace should just include the fennec project in the workspace and also generate that one.

> 
> @@ +58,5 @@
> > +        workspace_dir = self.get_workspace_path()
> > +        process = subprocess.check_call(['eclipse', '-data', workspace_dir])
> > +
> > +    def get_workspace_path(self):
> > +        # This must match python/mozbuild/mozbuild/backend/cpp_eclipse.py
> 
> Then refactor the calculation to exist in a reusable function.

How do I import from cpp_eclipse.py in this mach command?

Comment 6

[Benoit Girard (:BenWa)]

ni? on the above plus:

For your first concern I can suggest an alternative ./mach ide with a mandatory platform dependent argument. Say ./mach ide VisualStudio. Currently the build-backend -b CppEclipse is hard to remember and discover.

Comment 7

[Benoit Girard (:BenWa)]

ping

Comment 8

[Benoit Girard (:BenWa)]

Attachment: patch (fixed rot)

Comment 9

[Gregory Szorc [:gps]]

I agree that |build-backend -b <backend>| is not intuitive.

|mach ide| is not perfect because not all build backends are IDEs. But I agree |mach ide| is simpler.

I'll r+ the addition of |mach ide|. Worst case we consolidate build-backend and ide in the future. Bonus points if you update existing references. http://dxr.mozilla.org/mozilla-central/search?q=build-backend

Comment 10

[Benoit Girard (:BenWa)]

Attachment: patch v2

Comment 11

[Benoit Girard (:BenWa)]

I made a mandatory argument but I'm tempted to have a default of visualstudio on windows and eclipse (CppEclipse) elsewhere.

Comment 12

[Benoit Girard (:BenWa)]

Attachment: patch v3

Fixed issue with visualstudio launching.

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8508989 [details] [diff] [review]
patch v3

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

FWIW, I'd be quite pleased to make |mach eclipse| do the right thing on Android, which is a little more involved than what's needed for these two backends but would probably prevent a good deal of confusion.

Comment 14

[Benoit Girard (:BenWa)]

Cool, can you share what that would look like? Ideally 'mach eclipse' would be consistent and let you do platform development everywhere it's supported. It wouldn't be ideal for someone who uses 'mach eclipse' on mac to not get a platform project on android.

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 8508989 [details] [diff] [review]
patch v3

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

LGTM!

We could add some logic that ensured configure has executed, etc. But that's follow-up fodder.

::: python/mozbuild/mozbuild/backend/mach_commands.py
@@ +1,2 @@
> +from __future__ import print_function, unicode_literals
> +

Need MPL.

@@ +36,5 @@
> +        if backend == 'CppEclipse':
> +            try:
> +                which.which('eclipse')
> +            except which.WhichError:
> +                print('Eclipse CDT 8.4 or later must be installed in your PATH.\nDownload: http://www.eclipse.org/cdt/downloads.php')

Nit: Typically you do multiple print statements instead of embedding the \n.

Comment 16

[Benoit Girard (:BenWa)]

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

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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