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

configure: add --disable-windows-unicode option #2264

Merged
merged 1 commit into from Feb 27, 2019

Conversation

Projects
None yet
6 participants
@nojb
Copy link
Contributor

commented Feb 25, 2019

Adds a configure option to disable Unicode support under Windows. See MPR#7904.

@nojb nojb requested a review from shindere Feb 25, 2019

@avsm

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

As a CI maintainer who is slightly grumpy about the number of configure-time options we have to test in opam, could I gently ask what the usecase/impact for this toggle? Will this need to be tested on all submitted opam packages when we activate Windows CI with both runtime options? Or is the flag available for advanced users only who know what they are doing wrt Windows unicode?

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

As a CI maintainer who is slightly grumpy about the number of configure-time options we have to test in opam, could I gently ask what the usecase/impact for this toggle?

The usecase is that Unicode support is not backwards compatible (think about strings that were stored in a DB in Latin-1 encoding which are retrieved and passed to functions now expecting UTF-8). In such environments, migration to Unicode mode needs to be carefully managed.

Will this need to be tested on all submitted opam packages when we activate Windows CI with both runtime options?

No, it doesn't need to be tested.

Or is the flag available for advanced users only who know what they are doing wrt Windows unicode?

Only for advanced users.

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

On second thoughts, I don't think it is a good idea to have this as an option in configure. If one needs this, better to edit the Makefile directly.

@nojb nojb closed this Feb 26, 2019

@shindere

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

@nojb nojb reopened this Feb 26, 2019

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

@nojb would you really mind reopening this?

OK, sure.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

@shindere

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

@dra27

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

I completely agree with @shindere that there should be nothing which requires editing the generated files from configure - it's highly unhygienic. There's also no need to test the Unicode stuff in CI - as @nojb points out, it is an advanced option for legacy support.

@avsm

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

Many thanks for the clarifications!

@dra27

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

For the implementation, there are several modes available here, and it would be better if we now supported them all properly. Unicode mode is always enabled, because we use the wide functions, so from a sort of hygiene-perspective, I don't think it should be referred to as disabling Unicode.

I would do this using AC_ARG_VAR perhaps called WINDOWS_UNICODE_MODE (WINDOWS_UNICODE_TRANSLATION_MODE is a bit too much of a mouthful?!) to reflect that, which would have a default value of compatible and should accept values of:

  • ansi - causes windows_unicode_enabled = 0 in runtime/win32.c (i.e. sets WINDOWS_UNICODE to 0)
  • nonstrict - windows_unicode_enabled = 1 and windows_unicode_strict = 0 (this is new)
  • compatible - windows_unicode_enabled = 1 and windows_unicode_fallback = 1 (that's the current default)
  • strict - windows_unicode_enabled = 1, windows_unicode_strict = 1 and windows_unicode_fallback = 0 (this is new)
@dra27

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

Incidentally, at this point, I'd have no issue with only the ansi and compatible values being implemented - i.e. we put the infrastructure in place to expose the other flags later, but don't require any build system alterations at this time.

@dra27

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

NB For @shindere, my comment above previously incorrect suggested a default of fallback when it should have been a default of compatible

@shindere

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

@shindere

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

@shindere

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

For the implementation, there are several modes available here, and it would be better if we now supported them all properly. Unicode mode is always enabled, because we use the wide functions, so from a sort of hygiene-perspective, I don't think it should be referred to as disabling Unicode.

I would do this using AC_ARG_VAR perhaps called WINDOWS_UNICODE_MODE (WINDOWS_UNICODE_TRANSLATION_MODE is a bit too much of a mouthful?!) to reflect that, which would have a default value of compatible and should accept values of:

  • ansi - causes windows_unicode_enabled = 0 in runtime/win32.c (i.e. sets WINDOWS_UNICODE to 0)
  • nonstrict - windows_unicode_enabled = 1 and windows_unicode_strict = 0 (this is new)
  • compatible - windows_unicode_enabled = 1 and windows_unicode_fallback = 1 (that's the current default)
  • strict - windows_unicode_enabled = 1, windows_unicode_strict = 1 and windows_unicode_fallback = 0 (this is new)

Wish granted. @shindere do you mind taking another look?

@dra27

dra27 approved these changes Feb 26, 2019

Copy link
Contributor

left a comment

Lovely, thank you!

@dra27

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

This strikes me as both safe and a good idea for 4.08.0 - @gasche, @damiendoligez ?

@gasche

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

Yes, I think this can go in 4.08.

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

Does it need a CHANGES entry?

@gasche

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

I think it's a good idea to have a Changes entry, to let other Windows users know that it exists.

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

I think it's a good idea to have a Changes entry, to let other Windows users know that it exists.

OK, I added one.

@nojb nojb force-pushed the nojb:autoconf_windows_unicode branch from 4935f9d to 3a508c5 Feb 26, 2019

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

Yes, this can go into 4.08.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

@dra27

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

It's a little bike-sheddy, but cf. #2266 (comment) regarding the Changes entry ... updating the documentation might be more worthwhile (not sure which bit...!)

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

I updated the Changes entry to remove the word "environment".

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

It's a little bike-sheddy, but cf. #2266 (comment) regarding the Changes entry ... updating the documentation might be more worthwhile (not sure which bit...!)

If I understand, it is "OK" to leave this Changes entry separate or should I put it together with the rest of #1200?

Is there a section in the manual that discusses the options to configure?

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

Also, this is to be squashed before merging.

@dra27

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

@nojb - my suggestion was that the GPR number could simply be added to GPR#2139 (the autoconf PR) on the basis that it's simply an addition to that work.

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

@nojb - my suggestion was that the GPR number could simply be added to GPR#2139 (the autoconf PR) on the basis that it's simply an addition to that work.

OK, will do.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

Nicolás Ojeda Bär (2019/02/27 09:00 +0000):

@nojb - my suggestion was that the GPR number could simply be added to GPR#2139 (the autoconf PR) on the basis that it's simply an addition to that work. OK, will do.
I'd prefere not, actually. I really like to have separate entries for GPRs when their authors or reviewers are distinct. To me, having a single entry in such cases tends to be not only non acucrate but also misleading.

OK, in that case this PR is ready to merge. I will squash, merge and cherry-pick shortly unless someone complains.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

@nojb nojb force-pushed the nojb:autoconf_windows_unicode branch from f7a371d to 60a7320 Feb 27, 2019

@nojb nojb merged commit 7dfbfc6 into ocaml:trunk Feb 27, 2019

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@nojb nojb deleted the nojb:autoconf_windows_unicode branch Feb 27, 2019

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

Cherry-picked to 4.08 as daf9f27

@shindere

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.