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.

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

Add ability to add source/resource folder outside of the project basedir using linked folder #1603

Merged
merged 1 commit into from
Feb 2, 2024
Merged

Add ability to add source/resource folder outside of the project basedir using linked folder #1603

merged 1 commit into from
Feb 2, 2024

Conversation

RoiSoleil
Copy link
Contributor

Add ability to add source/resource folder outside of the project basedir using linked folder

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 13, 2023 •
edited
Loading

Test Results

  214 files  ±0    214 suites  ±0   23m 22s ⏱️ +53s
  664 tests +1    654 ✅ +3  10 💤 ±0  0 ❌  - 1 
1 328 runs  +2  1 306 ✅ +4  22 💤 ±0  0 ❌  - 1 

Results for commit 8bd7698. ± Comparison against base commit 045f3cb.

♻️ This comment has been updated with latest results.

@RoiSoleil RoiSoleil force-pushed the master branch 2 times, most recently from f662785 to 3547584 Compare November 16, 2023 14:48
HannesWell
HannesWell reviewed Nov 24, 2023
View reviewed changes
Copy link
Contributor

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

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

Thank you for this PR.

In general it looks reasonable.

I couldn't try it out yet, but already have some remarks, see below.

Can you please rebase this on top of the current master?

org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/AbstractJavaProjectConfigurator.java Outdated Show resolved Hide resolved
org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/AbstractJavaProjectConfigurator.java Outdated Show resolved Hide resolved
org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/AbstractJavaProjectConfigurator.java Outdated Show resolved Hide resolved
org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/AbstractJavaProjectConfigurator.java Outdated
Comment on lines 514 to 522
String resourceCanonicalPath = resourceDirectory.getAbsolutePath();
try {
resourceCanonicalPath = resourceDirectory.getCanonicalPath();
} catch(IOException ex) {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I have not yet tried this out, but I'm not sure this is correct.
If resource.getDirectory() is a relative path (haven't checked if that can happen), then it will resolve against the running directory of the application, which will be something like the IDE location.
But here it should be resolved against the project directory, shouldn't it?

Furthermore if resourceDirectory.getAbsolutePath() is the fall back in case getCanonicalPath() throws, it should only be called in the catch block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When i checked resource.getDirectory() were only absolutePath no relativePath

@RoiSoleil
Copy link
Contributor Author

Thx for your kind review ! I would to the changes you ask for. Nice to see that PR are welcomed ;)

@RoiSoleil RoiSoleil closed this Nov 27, 2023
@RoiSoleil RoiSoleil reopened this Nov 27, 2023
@RoiSoleil RoiSoleil force-pushed the master branch 3 times, most recently from 051c886 to c311bd9 Compare December 6, 2023 16:03
@RoiSoleil RoiSoleil requested a review from HannesWell December 6, 2023 16:04
@RoiSoleil RoiSoleil force-pushed the master branch 2 times, most recently from 6acaeb6 to b7bc379 Compare December 7, 2023 13:36
@RoiSoleil
Copy link
Contributor Author

@HannesWell All tests are ok now. Can you review ?

@RoiSoleil RoiSoleil force-pushed the master branch 3 times, most recently from af4122e to 1de82f8 Compare December 11, 2023 21:45
@RoiSoleil
Copy link
Contributor Author

@HannesWell Do you have time for a review ?

@HannesWell
Copy link
Contributor

@HannesWell Do you have time for a review ?

Sorry I'm currently in the pre-christmas rush but will review this ASAP hopefully within the next week.

@RoiSoleil
Copy link
Contributor Author

Ok thx for the reply ;)

@RoiSoleil RoiSoleil force-pushed the master branch 2 times, most recently from 428282d to 7845e00 Compare December 28, 2023 21:35
@RoiSoleil
Copy link
Contributor Author

@HannesWell Could you take a look ?

@HannesWell
Copy link
Contributor

@HannesWell Could you take a look ?

Thank for your patience and sorry for the delay.

I had a coarse look and was wondering if the change in the m2e-tests submodule is still necessary/intended? If yes please open a PR for that in the tesla repository and resolve the conflicts.

I'll do a detailed review tomorrow, but I think many changes, if any will be required.

@HannesWell HannesWell force-pushed the master branch 2 times, most recently from 20ddad8 to c720015 Compare January 23, 2024 19:44
HannesWell
HannesWell reviewed Jan 23, 2024
View reviewed changes
Copy link
Contributor

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

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

The change looks almost good (one point below).

I have applied a few minor simplifications.

@RoiSoleil please let me know if the changes in the m2e-tests submodule are still necessary.

org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/AbstractJavaProjectConfigurator.java Show resolved Hide resolved
@HannesWell HannesWell force-pushed the master branch 2 times, most recently from 9411e25 to 3ec363d Compare January 23, 2024 22:37
HannesWell
HannesWell approved these changes Jan 23, 2024
View reviewed changes
Copy link
Contributor

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

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

Given the tests pass, this is now fine for me.
@RoiSoleil are you fine with it as well?

@RoiSoleil
Copy link
Contributor Author

RoiSoleil commented Jan 25, 2024 •
edited
Loading

Given the tests pass, this is now fine for me. @RoiSoleil are you fine with it as well?

I tested it on my use case and it's ok. Thx for your time by the way ;)

@HannesWell HannesWell force-pushed the master branch 2 times, most recently from 92dec06 to eb52708 Compare January 27, 2024 21:20
@RoiSoleil
Copy link
Contributor Author

@HannesWell Do you need help to solde the failing test ?

@HannesWell
Copy link
Contributor

@HannesWell Do you need help to solde the failing test ?

Thanks for the offer. I've just looked into it.
The problem was the handling of absolute paths pointing to the project-basedir in the getFolder() method. I have now reworked this again to return the IProject in that case (which also allows to restore the prvious handling of that case in addResourceDirs()).

The previously failing test was fixed with that, lets see if all other pass as well. 🤞🏽

@HannesWell HannesWell force-pushed the master branch 2 times, most recently from 3f0cc95 to fd632d2 Compare February 2, 2024 18:42
@RoiSoleil @HannesWell
Add ability to add source/resource folder outside of the project
8bd7698 
Such source/resource folders outside of the project are made accessible
using linked folder.

Furthermore fix handling of absolute paths pointing to project-directory
in AbstractJavaProjectConfigurator.getFolder().
HannesWell
HannesWell approved these changes Feb 2, 2024
View reviewed changes
Copy link
Contributor

@HannesWell HannesWell left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Finally the build is green.
Thank you @RoiSoleil for this contribution and your patience.

@HannesWell HannesWell merged commit 60e66ff into eclipse-m2e:master Feb 2, 2024
7 checks passed
@RoiSoleil
Copy link
Contributor Author

@HannesWell Thank you for your time and work on this PR.

@HannesWell HannesWell added this to the 2.6.0 milestone Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HannesWell HannesWell HannesWell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.6.0
Development

Successfully merging this pull request may close these issues.
2 participants
@RoiSoleil @HannesWell