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

Cannot load library in OMEdit from a directory with accented letters #8777

Closed
casella opened this issue Mar 28, 2022 · 36 comments · Fixed by #8992 or #9044
Closed

Cannot load library in OMEdit from a directory with accented letters #8777

casella opened this issue Mar 28, 2022 · 36 comments · Fixed by #8992 or #9044
Assignees
Labels
bug COMP/GUI/OMEdit Issue and pull request related to OMEdit
Milestone

Comments

@casella
Copy link
Contributor

casella commented Mar 28, 2022

Unpack the attached zip file, that contains two libararies in an accented letter directory.

When trying to open TestPackage/package.mo, I get:

[1] 00:13:40 Translation Error
[C:/dev/OM64bit/OMCompiler/Compiler/FrontEnd/ClassLoader.mo: 313:9-313:128]: Internal error loadCompletePackageFromMp failed for unknown reason: mp=D:/Temp/Lib èèè pack=TestPackage

[2] 00:13:40 Scripting Error
Failed to load package TestPackage () using MODELICAPATH D:/Temp/Lib èèè/.

When I try to open ExternalMedia 3.3.0/package.mo instead, I get

[1] 00:15:02 Grammar Warning
[D:/Temp/Lib èèè/ExternalMedia 3.3.0/package.mo: 2:1-23:18]: Media was referenced in the package.order file, but was not found in package.mo, Media/package.mo or Media.mo.

and an empty ExternalMedia package is shown in the Libraries Browser.

@casella casella added bug COMP/GUI/OMEdit Issue and pull request related to OMEdit labels Mar 28, 2022
@casella casella added this to the 1.19.0 milestone Mar 28, 2022
@casella casella added this to To do in 1.19.0 Release plan via automation Mar 28, 2022
@casella casella moved this from To do to In progress in 1.19.0 Release plan Apr 11, 2022
@casella
Copy link
Contributor Author

casella commented Apr 11, 2022

@adrpo has already implemented this feature using short directory names in #8573, for use by the package manager and linker/compiler. This also needs to be done when loading libraries in OMEdit.

@casella
Copy link
Contributor Author

casella commented May 2, 2022

Short directory names seem to be used now, since the error I get with 1.19.0-beta.1 uses them. However, the issue is not yet fixed. Please unzip the TestAccentedLetterDir.zip.

If you attempt to load TestLibrary/package.mo from OMEdit under Windows, you get

[1] 01:14:23 Translation Errore
[C:/OM119/OM64bit/OMCompiler/Compiler/FrontEnd/ClassLoader.mo: 334:9-334:128]: 
Internal error loadCompletePackageFromMp failed for unknown reason: mp=D:/Temp/TestAccentedLetterDir/Lib èèè pack=TestPackage

[2] 01:14:23 Scripting Errore
Failed to load package TestPackage () using MODELICAPATH D:/Temp/TestAccentedLetterDir/Lib èèè/.

while if you run omc test.mos, you get

false
"[C:/OM119/OM64bit/OMCompiler/Compiler/FrontEnd/ClassLoader.mo:334:9-334:128:writable]
Error: Internal error loadCompletePackageFromMp failed for unknown reason: mp=D:/Temp/TestAccentedLetterDir/Lib èèè pack=TestPackage
Error: Failed to load package TestPackage () using MODELICAPATH D:/Temp/TestAccentedLetterDir/Lib èèè/.
"

@adrpo, as I understand the class loader for Windows must also be fixed, otherwise it would be impossible to load libraries from accented directories at all.

@AnHeuermann
Copy link
Member

I changed omc_fopen. Do we need to use it in OMEdit as well? I did not change it there, because it uses different stuff around fopen.

@adeas31 Should we check wat OMEdit is using and if omc_fopen is usable there?

@adeas31
Copy link
Member

adeas31 commented May 18, 2022

The library loading error is from compiler so if you have changed the compiler to use omc_fopen then it should be good enough. OMEdit only uses fopen for its own log files.

@casella
Copy link
Contributor Author

casella commented May 19, 2022

@AnHeuermann, @adeas31 please agree on who can take care of this and please make sure it is also cherry-picked to maintenance/v1.19, so it will go into 1.19.0 as well. Thanks!

@AnHeuermann
Copy link
Member

I'll give it a go today.

@AnHeuermann
Copy link
Member

First issue is, that function getProgramFromStrategy is able to fail silently when getting a file name from a hash table. I re-actiavted the error message in that function:

"[D:/workspace/OpenModelica/OMCompiler/Compiler/FrontEnd/ClassLoader.mo:827:9-827:184:writable] Error: Internal error HashTable missing file D:/workspace/Testitestetst/issue-8777/TestAccentedLetterDir/Lib èèè/TestPackage/Media.mo - all entries include:
D:/workspace/Testitestetst/issue-8777/TestAccentedLetterDir/Lib èèè/TestPackage/package.mo

But the root problem is, that the string for the directory path changes from Lib èèè to Lib ├¿├¿├¿. Could be an encoding issue.


On Ubuntu unzip did extract the directory to .../Lib KKK/.... I had to change it manually but then there is still no issue on Ubuntu.

By the way, is the typo in Media.mo relevant?

with TestPackage; <---- within?
package Media
end Media;

@AnHeuermann
Copy link
Member

Okay, the issue goes way deeper than what I could change quickly.

When creating a mos script test.mos

print("Test: Lib èèè\n\n");

in Windows I get

# omc test.mos 
Test: Lib èèè

I can add the error message #8985 to the master.
@casella Do you want the error message in maintenance/v.19 as well?

But I don't think we can solve this issue fast.
I opened a new ticket for this: #8986

@casella
Copy link
Contributor Author

casella commented May 19, 2022

First issue is, that function getProgramFromStrategy is able to fail silently when getting a file name from a hash table. I re-actiavted the error message in that function:

"[D:/workspace/OpenModelica/OMCompiler/Compiler/FrontEnd/ClassLoader.mo:827:9-827:184:writable] Error: Internal error HashTable missing file D:/workspace/Testitestetst/issue-8777/TestAccentedLetterDir/Lib èèè/TestPackage/Media.mo - all entries include:
D:/workspace/Testitestetst/issue-8777/TestAccentedLetterDir/Lib èèè/TestPackage/package.mo

But the root problem is, that the string for the directory path changes from Lib èèè to Lib ├¿├¿├¿. Could be an encoding issue.

On Ubuntu unzip did extract the directory to .../Lib KKK/.... I had to change it manually but then there is still no issue on Ubuntu.

You can recreate the problem with Lib äää on your German keyboard.

By the way, is the typo in Media.mo relevant?

with TestPackage; <---- within?
package Media
end Media;

Of course it should be within, sorry. Anyway, we are not even getting to the point where that file is read from the file system.

@casella
Copy link
Contributor Author

casella commented May 19, 2022

Okay, the issue goes way deeper than what I could change quickly.

When creating a mos script test.mos

print("Test: Lib èèè\n\n");

in Windows I get

# omc test.mos 
Test: Lib èèè

Is this just a problem of graphical rendering of the output on the Windows terminal? In that case, I wouldn't bother.

I can add the error message #8985 to the master.

OK. Can be helpful for debugging.

@casella Do you want the error message in maintenance/v.19 as well?

I don't think getting an Internal Error on getProgramFromStrategy (BTW, what the hell does that function name mean?) helps end-user much.

But I don't think we can solve this issue fast. I opened a new ticket for this: #8986

I'm still not convinced that this is the issue. Getting a bad rendering of pathnames with accented letters in error messages on a Windows cmd console is slightly annoying, but no big deal. Not being able to load libraries from directories with accented letters can completely break the package manager, if the username contains accented letters (or Russian characters, as it happened once with one of my students). That is a much bigger deal, particularly in corporate environments, where you cannot install stuff where you want in the file system, but you must stick to your home directory.

@casella
Copy link
Contributor Author

casella commented May 19, 2022

I mean, the package manager and the linker work fine in that case. We just need to use the same mechanism for the package loader.

@perost
Copy link
Member

perost commented May 19, 2022

I don't think getting an Internal Error on getProgramFromStrategy (BTW, what the hell does that function name mean?) helps end-user much.

It loads a file (as an Absyn.Program) using a given strategy: on-demand (loads directly from filesystem) or hashtable (loads from a hash table of pre-loaded files). It actually makes sense in context.

@sjoelund
Copy link
Member

I wonder if ANTLR internally loads things fine. Does loadFile work with accents in paths?

input = antlr3AsciiFileStreamNew(fName);

It might also exist other functions that do not support this as Windows is a mess.

@AnHeuermann
Copy link
Member

loadFile is working correctly in the case below:

loadFile("Lib èäöüß/model.mo"); getErrorString();

What is the difference in running the original problem? We do stuff differently for files named package.mo.

But I can't debug this without #8986 being fixed because my prints won't show the right stuff and I don't have Eclipse with MDT set up on my Windows system.

@AnHeuermann
Copy link
Member

AnHeuermann commented May 20, 2022

@mahge and myself tracked it down to void* System_moFiles(const char *directory). Using FindFirstFileW on unicode string pattern will solve the issue. I'll create a PR for it.

@AnHeuermann
Copy link
Member

I'll create the cherry pick for maintenance/v1. 19 on Monday.

@casella
Copy link
Contributor Author

casella commented May 20, 2022

I'd keep this open until we've actually tested it on 1.19.0-dev.beta5 😅

@casella casella reopened this May 20, 2022
@casella casella added this to the 1.19.1 milestone May 31, 2022
@casella casella removed this from In progress in 1.19.0 Release plan May 31, 2022
@casella casella modified the milestones: 1.19.1, 1.19.0 May 31, 2022
mahge added a commit to mahge/OpenModelica that referenced this issue May 31, 2022
  - Implement wide character handling for `SystemImpl__directoryExists`
    and `System_subDirectories`.

  - Remove the macros for conversion to/from wide character string, e.g,
    `MULTIBYTE_TO_WIDECHAR_LENGTH`, `MULTIBYTE_TO_WIDECHAR_VAR` ...
    and replace them with corresponding functions.

  - Added functions `omc_multibyte_to_wchar_str` and `omc_wchar_to_multibyte_str`
    to handle the conversions.
    Note: the caller has to deallocated the memory for the strings returned
    from these functions.

  - define `omc_stat_t` to be `stat` or `_stat` depending on platform and use
    it instead. We might need to change this to wstat on Windows later.

  - A few more minor cleanups and restructures.

  - Fixes OpenModelica#8777.
mahge added a commit to mahge/OpenModelica that referenced this issue May 31, 2022
  - Implement wide character handling for `SystemImpl__directoryExists`
    and `System_subDirectories`.

  - Remove the macros for conversion to/from wide character string, e.g,
    `MULTIBYTE_TO_WIDECHAR_LENGTH`, `MULTIBYTE_TO_WIDECHAR_VAR` ...
    and replace them with corresponding functions.

  - Added functions `omc_multibyte_to_wchar_str` and `omc_wchar_to_multibyte_str`
    to handle the conversions.
    Note: the caller has to deallocated the memory for the strings returned
    from these functions.

  - define `omc_stat_t` to be `stat` or `_stat` depending on platform and use
    it instead. We might need to change this to wstat on Windows later.

  - A few more minor cleanups and restructures.

  - Fixes OpenModelica#8777.
mahge added a commit to mahge/OpenModelica that referenced this issue May 31, 2022
  - Implement wide character handling for `SystemImpl__directoryExists`
    and `System_subDirectories`.

  - Remove the macros for conversion to/from wide character string, e.g,
    `MULTIBYTE_TO_WIDECHAR_LENGTH`, `MULTIBYTE_TO_WIDECHAR_VAR` ...
    and replace them with corresponding functions.

  - Added functions `omc_multibyte_to_wchar_str` and `omc_wchar_to_multibyte_str`
    to handle the conversions.
    Note: the caller has to deallocated the memory for the strings returned
    from these functions.

  - define `omc_stat_t` to be `stat` or `_stat` depending on platform and use
    it instead. We might need to change this to wstat on Windows later.

  - A few more minor cleanups and restructures.

  - Fixes OpenModelica#8777.
mahge added a commit to mahge/OpenModelica that referenced this issue May 31, 2022
  - Implement wide character handling for `SystemImpl__directoryExists`
    and `System_subDirectories`.

  - Remove the macros for conversion to/from wide character string, e.g,
    `MULTIBYTE_TO_WIDECHAR_LENGTH`, `MULTIBYTE_TO_WIDECHAR_VAR` ...
    and replace them with corresponding functions.

  - Added functions `omc_multibyte_to_wchar_str` and `omc_wchar_to_multibyte_str`
    to handle the conversions.
    Note: the caller has to deallocated the memory for the strings returned
    from these functions.

  - define `omc_stat_t` to be `stat` or `_stat` depending on platform and use
    it instead. We might need to change this to wstat on Windows later.

  - A few more minor cleanups and restructures.

  - Fixes OpenModelica#8777.
mahge added a commit that referenced this issue May 31, 2022
  - Implement wide character handling for `SystemImpl__directoryExists`
    and `System_subDirectories`.

  - Remove the macros for conversion to/from wide character string, e.g,
    `MULTIBYTE_TO_WIDECHAR_LENGTH`, `MULTIBYTE_TO_WIDECHAR_VAR` ...
    and replace them with corresponding functions.

  - Added functions `omc_multibyte_to_wchar_str` and `omc_wchar_to_multibyte_str`
    to handle the conversions.
    Note: the caller has to deallocated the memory for the strings returned
    from these functions.

  - define `omc_stat_t` to be `stat` or `_stat` depending on platform and use
    it instead. We might need to change this to wstat on Windows later.

  - A few more minor cleanups and restructures.

  - Fixes #8777.
@mahge
Copy link
Contributor

mahge commented May 31, 2022

This should be now fixed with #9044. We probably need to do more of these adjustments in more places. For now we can take them as they occure.

Please reopen if the issue is still there.

@casella
Copy link
Contributor Author

casella commented May 31, 2022

I'll test it tomorrow with the nightly build and re-close if it is fine. If it works for me, then it would be nice to backport the fix to maintenance/v1.19, so we get it in 1.19.1. Thanks!

@casella casella reopened this May 31, 2022
@casella casella modified the milestones: 1.19.1, 1.19.2 Jun 4, 2022
@casella
Copy link
Contributor Author

casella commented Jun 5, 2022

@mahge, I tested #9044 with the latest nightly and now everything works for me 😃

Could you please port it to maintenance/v1.19? Thanks!

mahge added a commit to mahge/OpenModelica that referenced this issue Jun 6, 2022
  - Implement wide character handling for `SystemImpl__directoryExists`
    and `System_subDirectories`.

  - Remove the macros for conversion to/from wide character string, e.g,
    `MULTIBYTE_TO_WIDECHAR_LENGTH`, `MULTIBYTE_TO_WIDECHAR_VAR` ...
    and replace them with corresponding functions.

  - Added functions `omc_multibyte_to_wchar_str` and `omc_wchar_to_multibyte_str`
    to handle the conversions.
    Note: the caller has to deallocated the memory for the strings returned
    from these functions.

  - define `omc_stat_t` to be `stat` or `_stat` depending on platform and use
    it instead. We might need to change this to wstat on Windows later.

  - A few more minor cleanups and restructures.

  - Fixes OpenModelica#8777.
mahge added a commit that referenced this issue Jun 6, 2022
  - Implement wide character handling for `SystemImpl__directoryExists`
    and `System_subDirectories`.

  - Remove the macros for conversion to/from wide character string, e.g,
    `MULTIBYTE_TO_WIDECHAR_LENGTH`, `MULTIBYTE_TO_WIDECHAR_VAR` ...
    and replace them with corresponding functions.

  - Added functions `omc_multibyte_to_wchar_str` and `omc_wchar_to_multibyte_str`
    to handle the conversions.
    Note: the caller has to deallocated the memory for the strings returned
    from these functions.

  - define `omc_stat_t` to be `stat` or `_stat` depending on platform and use
    it instead. We might need to change this to wstat on Windows later.

  - A few more minor cleanups and restructures.

  - Fixes #8777.
@mahge
Copy link
Contributor

mahge commented Jun 6, 2022

Fixed in #9044 and ported to maintenance/v1.19 in #9063.

@mahge mahge closed this as completed Jun 6, 2022
@casella
Copy link
Contributor Author

casella commented Jun 7, 2022

I prefer to keep this open until I've actually checked in on the built version, just in case. I'll take care of that.

@casella casella reopened this Jun 7, 2022
@casella casella added this to Check in 1.19.2-dev.beta1 in 1.19.2 release plan Jun 7, 2022
@casella
Copy link
Contributor Author

casella commented Jun 7, 2022

@casella casella closed this as completed Jun 23, 2022
@casella
Copy link
Contributor Author

casella commented Jul 5, 2022

Works fine in 1.19.2-dev.beta2.

@casella casella moved this from Check in 1.19.2-dev.beta1 to Done in 1.19.2 release plan Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug COMP/GUI/OMEdit Issue and pull request related to OMEdit
Projects
No open projects
7 participants