bugGNU Octave - Bugs: bug #49685, Set .m file encoding on a...

 
 

bug #49685: Set .m file encoding on a per-directory basis

Submitted by:  Markus Mützel <mmuetzel>
Submitted on:  Wed 23 Nov 2016 06:21:14 PM UTC  
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Unexpected Error or Warning
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  Closed
Release:  dev Operating System:  Any

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Fri 23 Oct 2020 12:01:33 PM UTC, comment #24: 

I set the mfile encoding to "system" (which is "CP1252" on my Windows locale) and ran the following commands (from patch #8696):

setenv('GNUTERM', 'qt')
graphics_toolkit gnuplot
h = text (0.5,0.6, 'H \0 \Gamma \aleph I')
set(h,'fontweight', 'bold')

The UTF-8 encoded letters showed correctly besides the current global mfile encoding being different.
So this change seems to work as expected.

Closing as fixed.

Markus Mützel <mmuetzel>
Project Member
Sat 17 Oct 2020 12:00:09 PM UTC, comment #23: 

I pushed the change here:
https://hg.savannah.gnu.org/hgweb/octave/rev/dff830c84726

Marking as ready for test.

Markus Mützel <mmuetzel>
Project Member
Wed 07 Oct 2020 06:37:55 PM UTC, comment #22: 

Looks I forgot to attach the patch.

Here it is.

(file #49932)

Markus Mützel <mmuetzel>
Project Member
Fri 02 Oct 2020 03:54:52 PM UTC, comment #21: 

The attached patch implements the approach outlined in comment #20.
It would still be possible to call `dir_encoding` from a PKG_ADD script (with the previously described pitfalls).
But the preferred way would be to have a file named `.oct_config` in the same folder with the .m files. That file would contain a line defining the encoding used in these .m files.
E.g.:

encoding=iso8859-1

I believe the patch is now ready for test.

Markus Mützel <mmuetzel>
Project Member
Mon 28 Sep 2020 05:38:06 PM UTC, comment #20: 

If the encoding that is used to read some .m file functions is changed, I see no way around re-parsing them (which is essentially the same as `clear THAT_FUNCTION`). That entails resetting persistent variables

After the first attempts on this, I'm starting to have doubts if using the `PKG_ADD` script is the best way to go ahead anyway. I like the approach of getting the file encoding when a directory is added to the path. That way the dir_encoding can be read and stored only ones instead of trying to get that information for each file that is parsed.
But when this information is provided with `PKG_ADD`, what seems to be happening is:
1. Parse all files in the new directory (including PKG_ADD).
2. Execute the PKG_ADD script (which might call `dir_encoding`).
3. Parse all files in the new directory again if the encoding has changed. (This has to be done manually with the current patch.)

Parsing the functions twice is not very efficient. And parsing the files with the wrong encoding in step 1 might lead to unexpected side effects (parse errors). Having functions that `mlock` themselves might also be problematic with that approach.

It would probably be better imho if the following happened when a new directory is added to the load path:
1. Look for a special file (that doesn't need to be parsed by Octave) in the new directory. Read and set the `dir_encoding` from information gathered from that file.
2. Parse all files with the correct encoding.
3. Potentially call the `PKG_ADD` script (or do whatever we decide elsewhere).

That's close to what Andrew proposed in comment #14.

Maybe that file could be named `.oct_settings`. That way we could add other directory specific settings in the future. (No idea what that could be. But who knows...)
The file itself could contain strings (one setting per line) that are easy to parse.
E.g.:

encoding=UTF-8

Does that approach look reasonable?

Markus Mützel <mmuetzel>
Project Member
Mon 28 Sep 2020 04:51:38 PM UTC, comment #19: 

Another issue (and one that we might want to discuss separately from this bug report) is whether we want to discourage the use of PKG_ADD files and possibly eliminate them in the future.  My concern is that PKG_ADD files may contain arbitrary code, so an addpath command could potentially do some damage.  At least this requires addpath and not a simple change of working directory (PKG_ADD is not loaded for the current directory) but that doesn't eliminate the overall problem.

John W. Eaton <jwe>
Project Administrator
Mon 28 Sep 2020 04:46:36 PM UTC, comment #18: 

I don't think there is currently a way to force all functions defined in a particular directory to be reloaded.

Also, remember that when functions are reloaded, persistent variables will be re-initialized.  Is that OK in this instance?

John W. Eaton <jwe>
Project Administrator
Sun 27 Sep 2020 07:20:14 PM UTC, comment #17: 

Is there an easy way to trigger re-parsing all functions in one folder (preferably including those in their private, @class and +namespace subfolders)? Either in C++ or .m file code.

`interpreter::clear_functions` clears all functions. But it would probably be more efficient to only re-parse those functions for which we know the encoding changed.

Markus Mützel <mmuetzel>
Project Member
Fri 18 Sep 2020 07:44:34 PM UTC, comment #16: 

The attached patch applies the directory specific m-file encoding to all files that would be added by an addpath command (including the special subfolders).
It also adds some documentation to the manual.

It should be mostly complete now.

I've only tested on Ubuntu. But I hope it equally works on other platforms.

If this were to be added to Octave, it would probably make sense to add PKG_ADD scripts to all folders in the repository that contain m-files. The directory specific m-file encoding should be set to UTF-8 in those files.

(file #49815)

Markus Mützel <mmuetzel>
Project Member
Fri 18 Sep 2020 06:16:26 PM UTC, comment #15: 

That's almost exactly what I was planning to do. I'll post an updated patch soon.

But instead of using a new `.encoding` file, I believe it would be ok to use the PKG_ADD script that already does what we need.
A user would add a PKG_ADD files in the folders they plan on adding to Octave's load path. That file would contain a line similar to the one in comment #10.

Markus Mützel <mmuetzel>
Project Member
Fri 18 Sep 2020 06:07:01 PM UTC, comment #14: 

Here's an idea: instead of doing a "recursive, overrideable" thing that would require traversing the directory tree, you could require that an `.encoding` file must be present at the exact directory level that's added as a `PATH` entry (or you do one source_encoding() function call per PATH entry), and it applies to the entire directory subtree below that path entry; no overrides allowed. You want to combine source trees with different encodings, you need to add them to the Octave `PATH` as separate entries. That way it costs you just one file access per directory tree, no matter  how deep the +package or `private/` hierarchy goes, and there's still a way to have sets of code with different encodings.

(Maybe this is something that was already discussed.)

Andrew Janke <apjanke>
Fri 18 Sep 2020 06:02:28 PM UTC, comment #13: 

Yeah, there's always performance to consider, and disk IO is a big deal.

Andrew Janke <apjanke>
Fri 18 Sep 2020 05:11:47 PM UTC, comment #12: 

Thanks for your feedback. Much appreciated.

Having the encoding apply to private, @class or (nested) +namespace folders that are made "available" by the addpath command is probably what a user would expect. I agree.

But we already agreed on the mailing list that we don't want to traverse the file tree to find some magic files in a "random" parent folder.

Markus Mützel <mmuetzel>
Project Member
Fri 18 Sep 2020 05:03:42 PM UTC, comment #11: 

> I'm unsure if the dir_encoding should also apply to any private, @class or (nested) +namespace folders in that directory.


I'd say yes: I'd think it should apply to the entire directory hierarchy. Who in their right mind is going to mix different file encodings based on whether a file is inside a package/private or not? That wouldn't work under Octave's current behavior, so there's no back-compatibility concerns here.

> On the other hand, it's probably also conceivable that a user would like to drop in a +namespace folder from another project that uses a different encoding.


That's a terrible way of composing software, and something we should discourage. ;) If you want to pull in another library, don't stick it inside the source tree for your own library's M-code; put it in a `lib/` dir somewhere and reference it. I realize it's something that people do with Matlab/Octave software sometimes, because most M-code libraries are just a pile of functions in the global namespace, so sometimes people hack it by sticking it in a `private/`, but that's never gonna work robustly, and I'd suggest discouraging that practice and trying to provide alternatives instead of molding the low-level behavior of Octave to support those kludges.

(Sticking another library's files under a +package directory without modification doesn't work, because then those functions and classes are not referenceable by their original unqualified names, which is probably what the library's internal code is doing. So that's not a case we need to support.)

If this is something we really want to support, I'd suggest having a `.encoding` file in a directory that takes effect for the entire directory subtree underneath it, which then could be overridden by an `.encoding` file in a subdirectory. Kind of like how `.editorconfig` and `.gitignore` files can override each other, on a subtree basis.

Andrew Janke <apjanke>
Fri 18 Sep 2020 04:51:04 PM UTC, comment #10: 

I'm unsure if the dir_encoding should also apply to any private, @class or (nested) +namespace folders in that directory.

One the one hand, it might make it easier for the user to write the necessary PKG_ADD file if those were included automatically. Something like this would suffice:

dir_encoding (fileparts (mfilename ("fullpath")), "iso8859-1");

On the other hand, it's probably also conceivable that a user would like to drop in a +namespace folder from another project that uses a different encoding. For this to work, it would be best if the PKG_ADD file contained a line for each (automatically loaded) subfolder.

Markus Mützel <mmuetzel>
Project Member
Fri 18 Sep 2020 03:59:26 PM UTC, comment #9: 

The attached patch adds the function dir_encoding (DIR, ENCODING) that can be used to override the mfile encoding setting for a certain directory.

I did some preliminary tests with a UTF-8 encoded script containing some non-ASCII characters:

>> tst_umlaut

Test mit Umlauten: äöüß
>> a=dir_encoding('.', 'iso8859-1')

a = utf-8
>> clear tst_umlaut

>> tst_umlaut

Test mit Umlauten: äöüÃ

It might be necessary to add some tilde expand, file name canonicalization or other things necessary to get reliable file paths.
It probably also needs some logic for +namespaces, @class and private folders.
But it should be ready for some initial tests.

Feedback e.g. about the function interface would be very welcome.

(file #49809)

Markus Mützel <mmuetzel>
Project Member
Fri 18 Sep 2020 01:48:49 PM UTC, comment #8: 

Ah, gotcha. Okay, that makes sense.

Andrew Janke <apjanke>
Fri 18 Sep 2020 01:39:48 PM UTC, comment #7: 

The PKG_ADD script is executed for each path that is added to the Octave search path. "pkg" just happens to use that feature. But it isn't limited to Octave packages.

Markus Mützel <mmuetzel>
Project Member
Fri 18 Sep 2020 12:48:34 PM UTC, comment #6: 

> But I agree that the new goal of adding a function that can set the .m file encoding on a per-directory basis is better.


IMHO a function and PKG_ADD hooks for Octave Forge packages isn't the best approach: encoding is a property of a file or sets of files on disk, and it'd be better to express that declaratively with sidecar files that live next to them in the same file hierarchy, or some internal encoding indicator that's embedded in the files, instead of requiring procedural manipulation, especially if that procedural manipulation is tied into `pkg` packaging insteapd of being a property of arbitrary Octave source trees.

Andrew Janke <apjanke>
Fri 18 Sep 2020 11:04:28 AM UTC, comment #5: 

The scope has somewhat shifted from my original report.

But I agree that the new goal of adding a function that can set the .m file encoding on a per-directory basis is better. That function could be called from the PKG_ADD script.

That is also what seems to be the conclusion of the mailing list thread.

Changing the title and tags accordingly.

Markus Mützel <mmuetzel>
Project Member
Sat 09 Mar 2019 12:04:20 PM UTC, comment #4: 

Here's one more edge case: Octave's BISTs can be defined in C++ *.cc source files in special %! lines inside C++ comments. How should they be interpreted?

E.g. prospective patch http://hg.savannah.gnu.org/hgweb/octave/rev/ccea3574f36b contains a test with "abc,äöü" strings.

Andrew Janke <apjanke>
Tue 05 Mar 2019 05:11:34 AM UTC, comment #3: 

Here's an alternative, if more complicated, idea that could support both use cases:

How about adding support for an explicit encoding indicator stored in a ".encoding" file in source code directories? For source files under dirs with an explicit encoding, that encoding would always be used for those files, regardless of what the Text Editor default encoding is. Then leave the Text Editor default encoding as System on Windows. (This would affect both the Text Editor and Octave's M-code interprerter.)

That way Windows Octave and Windows Matlab projects without explicit encodings would be compatible (as long as they're used on machines with the same system locale). And Octave-only projects that needed cross-OS or cross-locale portability could be given .encoding files (and use UTF-8).

Octave's core files and Octave Forge packages would all get UTF-8 .encoding files so they're portable.

See my big long post at http://lists.gnu.org/archive/html/octave-maintainers/2019-03/msg00019.html.

It's a bummer that this approach requires explicit action on the part of the user to achieve portability. But I think this might be necessary if you want both cross-OS/locale and with-Matlab compatibility.

Andrew Janke <apjanke>
Wed 23 Nov 2016 09:18:33 PM UTC, comment #2: 

See also bug #45597. I find it hard to decide what is the more important usecase for Windows Octave:

  • exchange m-files with Windows Matlab (That's why the default encoding in Windows is currently "system". That's "my" usecase.)
  • exchange m-files with Linux Octave, or with Windows Octave of other locales (proposal in this bug report, could be best covered with "utf8" encoding).
Hartmut <hardy>
Wed 23 Nov 2016 06:26:06 PM UTC, comment #1: 

Attached is a patch that makes the proposed change.

(file #39059)

Markus Mützel <mmuetzel>
Project Member
Wed 23 Nov 2016 06:21:14 PM UTC, original submission:  

Octave has a hard life with respect to international (non-ASCII) characters on Windows and there are a lot of (partly overlapping) open bug reports covering many different issues.
It would make things easier if we used only one encoding consistently all over Octave.

At the moment, the standard encoding for the text editor is set to "System" on Windows. That points to a lot of different codepages dependent on the locale of Windows (and its standard codepage). UTF-8 is the default codepage on non-Windows systems. This makes it difficult to exchange Octave files containing non-ASCII characters between different platform (or different locales of Windows).

UTF-8 can be used to encode any glyph that is available in Unicode.
Can we also make it the default on Windows?

Markus Mützel <mmuetzel>
Project Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #49932:  bug49685_dir_encoding_v3.patch added by mmuetzel (15KiB - application/octet-stream)
file #49815:  bug49685_dir_encoding_v2.patch added by mmuetzel (10KiB - application/octet-stream)
file #49809:  bug49685_dir_encoding.patch added by mmuetzel (8KiB - application/octet-stream)
file #39059:  bug49685_editor_default_codepage.patch added by mmuetzel (865B - application/octet-stream)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by apjanke (Posted a comment)
  • -email is unavailable- added by apjanke (Add myself to CC list)
  • -email is unavailable- added by hardy (Posted a comment)
  • -email is unavailable- added by mmuetzel (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 16 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-10-23 mmuetzel StatusReady For Test => Fixed
        Open/ClosedOpen => Closed
    2020-10-20 mmuetzel Dependencies- => bugs #55195 is dependent
    2020-10-07 mmuetzel Attached File- => Added bug49685_dir_encoding_v3.patch, #49932
    2020-10-02 mmuetzel StatusIn Progress => Ready For Test
    2020-09-28 mmuetzel StatusPatch Submitted => In Progress
    2020-09-18 mmuetzel Attached File- => Added bug49685_dir_encoding_v2.patch, #49815
    2020-09-18 mmuetzel Attached File- => Added bug49685_dir_encoding.patch, #49809
        StatusConfirmed => Patch Submitted
    2020-09-18 mmuetzel CategoryGUI => Interpreter
        Item GroupOther => Unexpected Error or Warning
        StatusNone => Confirmed
        Operating SystemMicrosoft Windows => Any
        SummaryChange default codepage of text editor to UTF-8 on Windows => Set .m file encoding on a per-directory basis
    2019-03-05 apjanke Carbon-Copy- => Added floss@apjanke.net
    2016-11-23 mmuetzel Attached File- => Added bug49685_editor_default_codepage.patch, #39059

    Back to the top


    Powered by Savane 3.5