thSoft / elysium

LilyPond IDE for Eclipse
http://elysium.thsoft.hu
14 stars 3 forks source link

lilypond \include directive incorrectly shows error message #122

Closed dickvalentine closed 6 years ago

dickvalentine commented 8 years ago

\include directives in my .ly files cause eclipse/elysium to report errors inappropriately. For example the code:

\include "/Users/myself/Library/LilyFiles/mymacros.ly"

invariably shows as an error (red x on the left) and reports the error "Imported resource could not be found." Despite that the resource certainly exists, and is in fact found at compile time.

nittka commented 8 years ago

Thanks for the feedback. With respect to includes, we have made essential changes compared to the last released version. I think, this problem should be solved already, although I cannot validate that statement (absolute paths look different on for Windows). Also, I cannot say when a new Elysium version will be released (there is a pdf rendering blocker on Mac, which I cannot work on).

If compiling the file works, you can safely ignore any errors and warnings Elysium may complain about. (The next version will have a validation preference page allowing you to configure error levels)

nittka commented 8 years ago

@thSoft: could you check whether absolute includes as above work on Mac?

thSoft commented 8 years ago

Unfortunately, this is also reproducible on my Mac with 9e69410469ca38beba62c0a8fc523ae97c490d0e (current master).

nittka commented 8 years ago

So LilyPondImportUriResolver#resolve(resourceURI, importURI) does not work reliably for absolute references on unix based systems. This calls for unit tests for both Windows and Unix based URIs (relative and absolute). Also, I think this issue should definitely be addressed before the next release.

I would further refactor the UriResolver class and prepare the tests and then assign the ticket to you @thSoft for checking the result.

thSoft commented 8 years ago

Thank you in advance for looking into this! The problem is that ImportUriValidator.checkImportUriIsValid(EObject) calls EcoreUtil2.isValidUri after calling our resolver which I think does not support absolute URIs (it compares the URI with getResolvedImportUri(resource, uri) which returns platform:/Users/myself/Library/LilyFiles/mymacros.ly, which clearly does not make any sense).

dickvalentine commented 8 years ago

To be clear, relative addressing provokes the same (?) spurious error report. so,

\include "../../../../../Users/myself/Library/LilyFiles/mymacros.ly"

likewise complains about "resource not found"

On Sun, Sep 18, 2016 at 8:25 AM, Dénes Harmath notifications@github.com wrote:

Thank you in advance for looking into this! The problem is that ImportUriValidator.checkImportUriIsValid(EObject) calls EcoreUtil2.isValidUri after calling our resolver which I think does not support absolute URIs (it compares the URI with getResolvedImportUri(resource, uri) which returns platform:/Users/myself/Library/LilyFiles/mymacros.ly which clearly does not make any sense).

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/thSoft/elysium/issues/122#issuecomment-247854046, or mute the thread https://github.com/notifications/unsubscribe-auth/AVFSxu9wLBSs24YRNHQvAJydmwVRFimLks5qrVfpgaJpZM4J5eGj .

nittka commented 8 years ago

Thanks for letting me know - relative paths leaving the workspace have to be checked as well.

nittka commented 8 years ago

The includes really are a nightmare (even on Windows there were still problems - paths including white spaces and projects imported from some other location, i.e. absolute project location is not in the workspace).

@thSoft could you please try out the changes I have made. If the Oomph setup executes correctly, in the org.elysium.test/resources/includeResolution there should be a project populated with a score.ly and include.lys in different folders. Import that project into the runtime workspace. The score.template contains a placeholder which should be replace by the correct path in score.ly. All the commented out includes are expected to work (both Elysium link resolution and LilyPond compilation).

Currently, I have no idea how to test this automatically. Even the plain resolution mechanism (without determining whether the resolved URI is actually included in the workspace) does not work platform independent. java.net.URI#resolve behaves differently depending on the file/folder represented by the URI existing or not., i.e. resolving "included.ly" against "c:/my/nonexisting/path/" does not yield the expected result - the latter is not seen as a directory, wheres "c:/my/existing/path/" is. Also turning "/home/user/path/" into a file on a Windows maching adds a device ("c:/home/user/path"). Now tell me how to write a platform independent test one can actually understand...

thSoft commented 8 years ago

Thank you for the investigation, it must have cost a huge amount of time to learn the platform-specific quirks! I'm going to try this, but in the meantime, can't we have the integration test itself create the necessary files instead of the Oomph setup? Regarding platform-dependency, I can't think of a better way but include platform-dependent decisions in the tests itself (or maybe separate the platform-specific cases to parametric tests?). I don't have any idea about automating test execution on multiple platforms...

nittka commented 8 years ago

Actually, the project created by the Oomph setup was intended for manual tests. I then tried (inspired by the big integration test), whether the linking works within the test... In the final version, this test would be a plugin-test, actually creating the file structure - in the optimal case even with rename refactoring... For the moment, I would leave it as it is - as a proof of concept.

The nice thing about the current setup is, that the file content is already created with platform-specific includes.

nittka commented 6 years ago

I think, the complete linking/refactoring/URI-resolution has to be rewritten. The current implementation assumes that the Eclipse workspace locations and absolute file locations match well enough, which is the case often enough (folder structure in workspace and on disk). However, with linked folders the whole system breaks - and as shown by this issue, the current efforts to deal with particular cases are not perfect...

The correct approach would be to base all the linking/refactoring on the absolute file location alone. This will be quite an effort. My suggestion would be to release a new version first, in order to have a stable starting point. Migration to Java 9, a new Xtext version and an updated include-handling will be the next big tasks.

dickvalentine commented 6 years ago

To be clear, my experience with elysium on the Mac OSX platform is that a lilypond \include directive provokes a spurious error regardless of whether the file referenced is located absolutely or relatively. Further, in cases where lilypond source makes use of a "variable" defined in an \included file, that variable is flagged as unknown/undefined. Again, spuriously.

Have you considered the point of view that elysium's error reporting is overreaching in terms of what it can know about the semantics of a single source file? If an included file doesn't exist, or a variable is truly undefined, that information will be correctly manifested (and can present itself for resolution) when the lilypond compiler is invoked on the source.

The correct approach could be to remove excessive/incompetent error reporting from elysium and just let lilypond do its job.

sincerely,

On Tue, Feb 6, 2018 at 9:52 AM, Alexander Nittka notifications@github.com wrote:

I think, the complete linking/refactoring/URI-resolution has to be rewritten. The current implementation assumes that the Eclipse workspace locations and absolute file locations match well enough, which is the case often enough (folder structure in workspace and on disk). However, with linked folders the whole system breaks - and as shown by this issue, the current efforts to deal with particular cases are not perfect...

The correct approach would be to base all the linking/refactoring on the absolute file location alone. This will be quite an effort. My suggestion would be to release a new version first, in order to have a stable starting point. Migration to Java 9, a new Xtext version and an updated include-handling will be the next big tasks.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/thSoft/elysium/issues/122#issuecomment-363507503, or mute the thread https://github.com/notifications/unsubscribe-auth/AVFSxnVVmcaO9BYxlUMPfz0h3KJa0T-Iks5tSJFIgaJpZM4J5eGj .

nittka commented 6 years ago

Our points of view do not contradict each other at all. I hope for a quick release now!! You can then define the error level for Elysium include errors, even disabling them completely via the LilyPond Validation preference page. I think, this should resolve most of the symptoms bothering you - if not, please open further tickets.

However, unresolved includes also mean that linking and code completion for command calls will not work, breaking determining affected files for automatic re-compilation and file refactorings (rename, move). Elysium should do its best job at getting the linking as correct as possible, which is why the unterlying problem still has a high priority for me.

thSoft commented 6 years ago
nittka commented 6 years ago

Sorry for abusing the ticket for release planning ;-)

Yes, setting the defaults to ignore is fine with me. I have opened new tickets for the remaining issues discussed here, so I think we can now close this ticket without further directly related commits, @thSoft? Which branch do you want to merge after the release? In other words, in which order do we want to proceed now and after the release.

Both have their merit. Updating itself will be a major effort (without anything being "better" afterwards) and take some time. Would you consider merging the includeRefactoring branch before releasing (well, of course test it before releasing) or do you think the changes I tried there should be included wrt. tickets #132, #134.

thSoft commented 6 years ago

I'd rather close this one as https://github.com/thSoft/elysium/issues/122#issue-176112195 is resolved and start reviewing your branch after release.

Thanks for opening separate issues for the concerns I mentioned. Let's continue the discussion in those tickets.

dickvalentine commented 6 years ago

Should I assume your mention of "the LilyPond Validation preference page" refers to a capability not yet available to us regular users. I have elysium v. 0.5.2.201508091006, which seems to have been the latest available from eclipse marketplace until just a little earlier today. But I am not able to find any LilyPond Validation options.

My attempt to update to v. 0.0.0.201802071711 (just now) results in the following error response:

An error occurred while collecting items to be installed session context was:(profile=_Applications_Dev_java-oxygen_Eclipse.app_Contents_Eclipse, phase=org.eclipse.equinox.internal.p2.engine.phases.Collect, operand=, action=). No repository found containing: osgi.bundle,javax.util,0.0.0.201802071711 No repository found containing: osgi.bundle,org.eclipse.emf.util,0.0.0.201802071711 No repository found containing: osgi.bundle,org.eclipse.swt.util,0.0.0.201802071711 No repository found containing: osgi.bundle,org.eclipse.ui.views.file,0.1.0.201802071711 No repository found containing: osgi.bundle,org.eclipse.ui.views.midi,0.1.0.201802071711 No repository found containing: osgi.bundle,org.eclipse.ui.views.pdf,0.3.0.201802071711 No repository found containing: osgi.bundle,org.eclipse.util,0.0.0.201802071711 No repository found containing: osgi.bundle,org.elysium,0.5.3.201802071711 No repository found containing: osgi.bundle,org.elysium.branding,0.5.3.201802071711 No repository found containing: osgi.bundle,org.elysium.ui,0.5.3.201802071711 No repository found containing: osgi.bundle,org.jpedal,6.24.0 No repository found containing: org.eclipse.update.feature,org.eclipse.commons,0.3.0.201802071711 No repository found containing: org.eclipse.update.feature,org.elysium.feature,0.5.3.201802071711

thanks,

On Tue, Feb 6, 2018 at 10:08 PM, Alexander Nittka notifications@github.com wrote:

Our points of view do not contradict each other at all. I hope for a quick release now!! You can then define the error level for Elysium include errors, even disabling them completely via the LilyPond Validation preference page. I think, this should resolve most of the symptoms bothering you - if not, please open further tickets.

However, unresolved includes also mean that linking and code completion for command calls will not work, breaking determining affected files for automatic re-compilation and file refactorings (rename, move). Elysium should do its best job at getting the linking as correct as possible, which is why the unterlying problem still has a high priority for me.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/thSoft/elysium/issues/122#issuecomment-363667585, or mute the thread https://github.com/notifications/unsubscribe-auth/AVFSxkrwe70QCuUf44O0RbgKvRfW1ROxks5tST3mgaJpZM4J5eGj .

nittka commented 6 years ago

Yes, the validation preference page will be available with 0.5.3, which I hope will be available soon.

thSoft commented 6 years ago

I released 0.5.3. I tested both updating an existing installation and installing from scratch with Eclipse Mars and Oxygen. @dickvalentine, if updating/installing still fails, please create a separate issue and provide your system information (About Eclipse > Installation Details > Configuration) and the exact steps you performed there.