eclipse-platform / eclipse.platform.resources

Eclipse Public License 2.0
3 stars 18 forks source link

Move org.eclipse.ui.examples.filesystem to eclipse.platform.resources #110

Closed ghost closed 2 years ago

ghost commented 2 years ago

As suggested in eclipse-platform/www.eclipse.org-eclipse#1, I'd like to move the org.eclipse.ui.examples.filesystem project from www.eclipse.org-eclipse to this repository.

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-110/1/

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-110/2/

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-110/3/

vogella commented 2 years ago

LGTM, lets see that the CI thinks of this. This repo has some random tests errors (see issues) so tests errors are most likely not related.

ghost commented 2 years ago

I have removed almost all compiler warnings and the only ones remaining so far are due to the usage of org.eclipse.core.internal.filesystem.Messages/Policy.

I can't replace them with the Eclipse logger because they are used to construct CoreExceptions. Does anyone know of a good alternative? Would it be alright to simply ignore those warnings?

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-110/4/

vogella commented 2 years ago

Checking in now...

ghost commented 2 years ago

Can you set he project encoding (right mouse click on project and Properties) to UTC-8 and text file delimitor to Unix?

Done.

vogella commented 2 years ago

Can you set he project encoding (right mouse click on project and Properties) to UTC-8 and text file delimitor to Unix?

Done.

Cool, lets see if that makes CI happy.

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-110/5/

vogella commented 2 years ago

The build error looks unrelated to me, lets wait a bit maybe this is just a temporay hick-up. I check again tomorrow.

ghost commented 2 years ago

error: warnings found and -failOnWarning specified

I assume that's the problem, because the example (ZipFileStore, to be specific) is using some internal Eclipse classes for "logging" purposes.

I have already resolved most of the remaining warnings, but this one is something where I haven't found a good alternative yet. Do you have any suggestions?

vogella commented 2 years ago

error: warnings found and -failOnWarning specified

I assume that's the problem, because the example (ZipFileStore, to be specific) is using some internal Eclipse classes for "logging" purposes.

For logging you should use Platform.getLog(getClass()).warning / error / info -> written from memory, so the API might look at bit different.

ghost commented 2 years ago

error: warnings found and -failOnWarning specified

I assume that's the problem, because the example (ZipFileStore, to be specific) is using some internal Eclipse classes for "logging" purposes.

For logging you should use Platform.getLog(getClass()).warning / error / info -> written from memory, so the API might look at bit different.

I probably didn't explain the problem sufficiently. The class uses Policy.error(...) which looks like a normal logging method, but works like: if error, then throw CoreException Thinking about it more, I can replace the call by throwing the CoreException directly, which would remove at least that dependency.

There's also the problem of reusing the internal error messages stored in the 'org.eclipse.core.internal.filesystem' package. I've looked at how it is handled in other classes of the example, and there the reference has been replaced by a plain string. So I think I'll adapt that solution.

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-110/6/

vogella commented 2 years ago

I would avoid throwing exceptions if possible, better only to log. But as you said, you can always throw an exception if desired.

ghost commented 2 years ago

I would avoid throwing exceptions if possible, better only to log. But as you said, you can always throw an exception if desired.

In general, I'd agree. But I'd also like the modifications to the code to a minimum. The code right now is implemented with exceptions in mind. i.e. If throwing a CoreException is replaced with simple logging, then the project would often fail in the next line due to a NullPointerException. Hence why I keep them in there, for now.

I have removed the remaining warnings and the project is compiling. However, the build is instead failing when executing the tycho-eclipserun-plugin with the error:

Status ERROR: org.eclipse.pde.core code=0 Problems occurred while resolving the target contents children=[Status ERROR: org.eclipse.pde.core code=0 Problems loading repositories children=[Status ERROR: org.eclipse.pde.core code=0 Problems loading repositories children=[Status ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.eclipse.ui.examples.filesystem 0.0.0]]]

This seems to be caused when the api-check profile is active. More specifically, it constructs a target-definition containing only the current artifact, which fails, since the artifact hasn't been released yet:

<target name="${project.artifactId}-apiBaseline" sequenceNumber="1">
    <locations>
        <location includeAllPlatforms="false" includeConfigurePhase="false" includeMode="planner" includeSource="false" type="InstallableUnit">
            <repository location="https://download.eclipse.org/eclipse/updates/4.23/R-4.23-202203080310/"/>
            <unit id="org.eclipse.ui.examples.filesystem" version="0.0.0"/>
        </location>
    </locations>
</target>

It then runs the tycho-eclipserun-plugin using this definition file. I'm not exactly sure what the intent behind this step is, but I assume it is to check, whether the Eclipse can be started with this plugin? What would be the correct approach to solve this problem?

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-110/7/

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-110/8/

vogella commented 2 years ago

Thanks @pzi-dsa for moving and updating this example code.

As this is "just" an example I merge it, as example code does not affected M3.