Skip to content

Add an option to automatically update the BREE to the highest of all dependencies #1813 #1928

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add an option to automatically update the BREE to the highest of all dependencies #1813 #1928

wants to merge 1 commit into from

Conversation

@sderazo
Copy link

@sderazo sderazo commented Aug 11, 2025

This feature is available on the Organize Manifests Wizard as "Update required execution environments".

@sderazo sderazo force-pushed the update-bree-feature branch from cd98611 to b1e3858 Compare August 13, 2025 17:42
@merks
Copy link
Contributor

merks commented Aug 13, 2025

Merge commits cannot be accepted. Please rebase and force push.

@HannesWell
Copy link
Member

HannesWell commented Aug 13, 2025 •
edited
Loading

@sderazo please squash your changes into one commit locally, rebase on top of the latest master (of this this repo) and force push the resulting commit to the branch of this PR. Please DO NOT create yet another PR.
If you need help on that, please get in touch with your Mentor from CodeDays.

Furthermore please remove all unrelated changes e.g. in the plugin-based-with-automatic-add.launch file or files with only white-space changes or addition of commented out code. If you need these changes, please keep them locally for you in a separate commit.
With all these unrelated changes this PR is too difficult to review.

I assume this addresses:

@HannesWell HannesWell linked an issue Aug 13, 2025 that may be closed by this pull request
Add an option to automatically update the BREE to the highest of all dependencies #1813
Open
@sderazo
Copy link
Author

sderazo commented Aug 14, 2025

@merks @HannesWell Thank you for your feedback. I have a meeting scheduled with my Mentor for tomorrow to correct this. I apologize for the current state that this PR is in.

I also have a question for the eclipsefdn/eca check. My teammates @JoseRodriguez26 @GreetingsEarthling and I all have signed the Eclipse Contributor Agreement. However, there were a couple of commits that one of my teammates and I pushed from our own local machines, where the author is of the format [email protected] (for example). This caused the eclipsefdn/eca check to fail. With some research, I discovered a method to change the author of my commits with [email protected] using git filter-repo and implemented it. Was this the correct approach to take? I now realize I should have asked about the implementation before using it. My teammate has not yet changed the author of his commits with his local machine as the author to the email registered with Eclipse, and we would like to know how to correctly address this.

Thank you for your time.

@GreetingsEarthling @sderazo @JoseRodriguez26
Add an option to automatically update the BREE to the highest of all …
d828b90 
…dependencies eclipse-pde#1813

We added an option to automatically update the BREE to the highest of all dependencies to the Organize Manifests Wizard. It appears on the wizard as "Update required execution environments”. Instead of having to navigate to the manifest of each project to click the quick-fix because one of the dependencies has a higher BREE than declared in the manifest, this feature automatically updates the manifest to many projects.

Fixes eclipse-pde#1813

Co-authored-by: Sabrina Diaz-Erazo <[email protected]>
Co-authored-by: Jose Rodriguez <[email protected]>
@sderazo sderazo force-pushed the update-bree-feature branch from fbd6ac7 to d828b90 Compare August 15, 2025 20:22
@sderazo
Copy link
Author

sderazo commented Aug 15, 2025

@sderazo please squash your changes into one commit locally, rebase on top of the latest master (of this this repo) and force push the resulting commit to the branch of this PR. Please DO NOT create yet another PR. If you need help on that, please get in touch with your Mentor from CodeDays.

Furthermore please remove all unrelated changes e.g. in the plugin-based-with-automatic-add.launch file or files with only white-space changes or addition of commented out code. If you need these changes, please keep them locally for you in a separate commit. With all these unrelated changes this PR is too difficult to review.

addressed in commit d828b90

I assume this addresses:

Yes, that is correct

prashanthimatam
prashanthimatam reviewed Aug 22, 2025
View reviewed changes
...clipse.pde.core/src/org/eclipse/pde/internal/core/builders/execenv/ExecEnvironmentUtils.java
*/
public class ExecEnvironmentUtils extends JarManifestErrorReporter {

public ExecEnvironmentUtils(IFile file) {
Copy link

Choose a reason for hiding this comment

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

should not extend internal JarManifestErrorReporter, just make it a final static utility class

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/builders/BundleErrorReporter.java

try {
if (highestBundleEE != getHighestEE(highestDependencyEE, highestBundleEE)) {
if (highestBundleEE != ExecEnvironmentUtils.getHighestEE(highestDependencyEE, highestBundleEE)) {
Copy link

Choose a reason for hiding this comment

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

Looks good !

...clipse.pde.core/src/org/eclipse/pde/internal/core/builders/execenv/ExecEnvironmentUtils.java
"CDC-1.1/Foundation", "JRE", "J2SE", "JavaSE"); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$

private static final Pattern EE_PATTERN = Pattern.compile("(.*)-(\\d+)\\.?(\\d+)?(.*)?"); //$NON-NLS-1$

Copy link

Choose a reason for hiding this comment

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

There might be some failures try adding testcases for regex pattern

...clipse.pde.core/src/org/eclipse/pde/internal/core/builders/execenv/ExecEnvironmentUtils.java
}
if (eeMatcher2.groupCount() > 2 && eeMatcher2.group(3) != null) {
eeMinorVersion2 = Integer.valueOf(eeMatcher2.group(3));
}
Copy link

Choose a reason for hiding this comment

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

It works for need unit tests for malformed inputs and ordering issues

...clipse.pde.core/src/org/eclipse/pde/internal/core/builders/execenv/ExecEnvironmentUtils.java
* given
*/
public static String getHighestBREE(String[] executionEnvironments) {
if (executionEnvironments.length == 0) {
Copy link

Choose a reason for hiding this comment

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

make sure other functions receive empty string or null . Make sure there is uniformity

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/builders/BundleErrorReporter.java
} catch (Exception e) {
PDECore.log(e);
}
}
Copy link

Choose a reason for hiding this comment

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

Better not to return array object in the end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@prashanthimatam prashanthimatam prashanthimatam left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add an option to automatically update the BREE to the highest of all dependencies

5 participants

@sderazo @merks @HannesWell @prashanthimatam @GreetingsEarthling