Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 570760 - Option to automatically add requirements to product-launch #7

Merged
merged 1 commit into from
May 16, 2022

Conversation

HannesWell
Copy link
Member

This PR resolves Bug 570760

It was first addressed in Eclipse Gerrit Change 186834 but abandoned because it was not yet compelted at the time of the move.

@HannesWell HannesWell requested a review from jhonnen April 2, 2022 14:32
@HannesWell
Copy link
Member Author

Last comment from @laeubi at the mentioned Gerrit:

PDE is the consumer. If I create a product and check it into my code repository I don't like to tell the user he should set something in his launch config before launching the product ... This is an important part of the configuration and should be persisted.

I have not yet considered that point, but I agree with Christoph that this alone is a reason to persist the newly added attribute in the product, even tough we are not yet 100% certain that P2/Tycho will be able to handle this subsequently (but I think it is likely that this is possible).

Besides that I wonder if autoIncludeRequirements=true should be used as default for Plug-in and Feature based launches, because this is the ProductActions default for both cases, isn't it? I have to check that.

Copy link

@jhonnen jhonnen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure... Persisting that option is a good point, but that's basically what launch configurations are for.
To me the primary consumer for the product file is the product build, not the developer launching it locally. So I still would not add any option, that is not used by the product build in the near future.

But since PDE doesn't have a product build anymore and I'm not involved with Tycho, I'll leave that decision to you.
The code looks fine though (except for the unclear default).

@laeubi
Copy link
Contributor

laeubi commented Apr 4, 2022

Persisting that option is a good point, but that's basically what launch configurations are for.

Why having products at all then? We could simply use "OSGi Framework Launch" config then

To me the primary consumer for the product file is the product build not the developer

I literally don't know anyone writing product files but never launching them in the IDE, for my daliy development launching products is what I do all day, and I almost never change/edit run configs for that.

@HannesWell
Copy link
Member Author

I checked how Tycho/P2 is behaving and verified that even for plug-in based launches requirements are included automatically.
The code was updated accordingly.
Consequently the default is changed for plug-in based products.

I'm in favor to submit this change now. I already created an issue to support that flag in p2: eclipse-equinox/p2#76

The only thing I'm thinking about to change is the location of the Include requirements check-box. At the moment it is at the overview page next to the radio-buttons if the product is based on plug-ins or features. But I think it would fit better into the bottom of the contents tab.

@HannesWell
Copy link
Member Author

HannesWell commented May 15, 2022

The only thing I'm thinking about to change is the location of the Include requirements check-box. At the moment it is at the overview page next to the radio-buttons if the product is based on plug-ins or features. But I think it would fit better into the bottom of the contents tab.

I have done that. The Overview-page is now unchanged and the Contents page looks like the following:

  • For products based on Features:
    grafik
  • For products based on Plug-ins:
    grafik

Please not that I have updated the label of the check-box to include optional dependencies when adding required plug-ins to make it clear that this check box is only considered by the button on the right. Not when automatically including requirements.

The check-box Include required Plug-ins/Features automatically is enabled by default (if not attribute is present).
For Plug-in based Products that changes how products are launched from the IDE (but reflects how Tycho builds such products).
For feature based products this is also changed compared to the behavior of the 2022-03 release, not because of this change but because Feature-based launches are now created for feature based products.

Please let me know what you think about this change.
I plan to submit this latest tomorrow (Monday) evening so that it will be part of M3 (which I again expected one week later, somehow the milestones are always earlier than expected).

Change-Id: I6881ca000452d70558e434e2e8b06bff6e3e6dcd
Signed-off-by: Hannes Wellmann <wellmann.hannes1@gmx.net>
@HannesWell
Copy link
Member Author

No objections were raised, the build looks good and today is last day of development. So this is finally ready for submission.

@HannesWell HannesWell merged commit 99d1e82 into eclipse-pde:master May 16, 2022
@HannesWell HannesWell deleted the bug570760 branch May 16, 2022 18:40
@HannesWell HannesWell added this to the 4.24 M3 milestone May 16, 2022
@HannesWell
Copy link
Member Author

PR for the N&N entry:
eclipse-platform/www.eclipse.org-eclipse-news#24

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
noteworthy Noteworthy feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants