You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2017/01/11 23:37:35 UTC
Review Request 55435: Windows: Fixed the locale guard in jsonify.hpp.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55435/
-----------------------------------------------------------
Review request for mesos, Alexander Rojas, Daniel Pravat, Alex Clemmer, and Michael Park.
Repository: mesos
Description
-------
Changing locales in Windows is mainly accomplished via `setlocale`,
which takes a bit mask (with constants named slightly differently
than in the POSIX headers) and a `char*`.
Diffs
-----
3rdparty/stout/include/stout/jsonify.hpp 3c48046e087de2a66139a31449327fd94c149371
Diff: https://reviews.apache.org/r/55435/diff/
Testing
-------
Windows:
msbuild Mesos.sln /p:PreferredToolArchitecture=x64 /m /t:stout-tests
3rdparty\stout\tests\Debug\stout-tests.exe --gtest_filter="*Json*"
Thanks,
Joseph Wu
Re: Review Request 55435: Windows: Fixed the locale guard in
jsonify.hpp.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55435/#review161460
-----------------------------------------------------------
Ship it!
Ship It!
- Andrew Schwartzmeyer
On Jan. 12, 2017, 11:40 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55435/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2017, 11:40 p.m.)
>
>
> Review request for mesos, Alexander Rojas, Daniel Pravat, Alex Clemmer, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changing locales in Windows is mainly accomplished via `setlocale`,
> which takes a bit mask (with constants named slightly differently
> than in the POSIX headers) and a `char*`.
>
> This also adds a comment to the locale guard.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/jsonify.hpp 3c48046e087de2a66139a31449327fd94c149371
>
> Diff: https://reviews.apache.org/r/55435/diff/
>
>
> Testing
> -------
>
> Windows:
>
> msbuild Mesos.sln /p:PreferredToolArchitecture=x64 /m /t:stout-tests
> 3rdparty\stout\tests\Debug\stout-tests.exe --gtest_filter="*Json*"
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 55435: Windows: Fixed the locale guard in
jsonify.hpp.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55435/#review161470
-----------------------------------------------------------
Ship it!
Ship It!
- Michael Park
On Jan. 12, 2017, 3:40 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55435/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2017, 3:40 p.m.)
>
>
> Review request for mesos, Alexander Rojas, Daniel Pravat, Alex Clemmer, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changing locales in Windows is mainly accomplished via `setlocale`,
> which takes a bit mask (with constants named slightly differently
> than in the POSIX headers) and a `char*`.
>
> This also adds a comment to the locale guard.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/jsonify.hpp 3c48046e087de2a66139a31449327fd94c149371
>
> Diff: https://reviews.apache.org/r/55435/diff/
>
>
> Testing
> -------
>
> Windows:
>
> msbuild Mesos.sln /p:PreferredToolArchitecture=x64 /m /t:stout-tests
> 3rdparty\stout\tests\Debug\stout-tests.exe --gtest_filter="*Json*"
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 55435: Windows: Fixed the locale guard in
jsonify.hpp.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55435/
-----------------------------------------------------------
(Updated Jan. 12, 2017, 3:40 p.m.)
Review request for mesos, Alexander Rojas, Daniel Pravat, Alex Clemmer, and Michael Park.
Repository: mesos
Description
-------
Changing locales in Windows is mainly accomplished via `setlocale`,
which takes a bit mask (with constants named slightly differently
than in the POSIX headers) and a `char*`.
This also adds a comment to the locale guard.
Diffs
-----
3rdparty/stout/include/stout/jsonify.hpp 3c48046e087de2a66139a31449327fd94c149371
Diff: https://reviews.apache.org/r/55435/diff/
Testing
-------
Windows:
msbuild Mesos.sln /p:PreferredToolArchitecture=x64 /m /t:stout-tests
3rdparty\stout\tests\Debug\stout-tests.exe --gtest_filter="*Json*"
Thanks,
Joseph Wu
Re: Review Request 55435: Windows: Fixed the locale guard in
jsonify.hpp.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55435/#review161454
-----------------------------------------------------------
Bad patch!
Reviews applied: [55435, 55029, 55028, 55026, 55025, 55024, 55023, 55022]
Failed command: python support/apply-reviews.py -n -r 55022
Error:
2017-01-12 23:23:19 URL:https://reviews.apache.org/r/55022/diff/raw/ [1047/1047] -> "55022.patch" [1]
error: patch failed: 3rdparty/libprocess/src/io.cpp:79
error: 3rdparty/libprocess/src/io.cpp: patch does not apply
Full log: https://builds.apache.org/job/Mesos-Reviewbot/16704/console
- Mesos ReviewBot
On Jan. 12, 2017, 11:14 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55435/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2017, 11:14 p.m.)
>
>
> Review request for mesos, Alexander Rojas, Daniel Pravat, Alex Clemmer, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changing locales in Windows is mainly accomplished via `setlocale`,
> which takes a bit mask (with constants named slightly differently
> than in the POSIX headers) and a `char*`.
>
> This also adds a comment to the locale guard.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/jsonify.hpp 3c48046e087de2a66139a31449327fd94c149371
>
> Diff: https://reviews.apache.org/r/55435/diff/
>
>
> Testing
> -------
>
> Windows:
>
> msbuild Mesos.sln /p:PreferredToolArchitecture=x64 /m /t:stout-tests
> 3rdparty\stout\tests\Debug\stout-tests.exe --gtest_filter="*Json*"
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 55435: Windows: Fixed the locale guard in
jsonify.hpp.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55435/
-----------------------------------------------------------
(Updated Jan. 12, 2017, 3:14 p.m.)
Review request for mesos, Alexander Rojas, Daniel Pravat, Alex Clemmer, and Michael Park.
Changes
-------
Improved destruction of locale guard and added comments.
Repository: mesos
Description (updated)
-------
Changing locales in Windows is mainly accomplished via `setlocale`,
which takes a bit mask (with constants named slightly differently
than in the POSIX headers) and a `char*`.
This also adds a comment to the locale guard.
Diffs (updated)
-----
3rdparty/stout/include/stout/jsonify.hpp 3c48046e087de2a66139a31449327fd94c149371
Diff: https://reviews.apache.org/r/55435/diff/
Testing
-------
Windows:
msbuild Mesos.sln /p:PreferredToolArchitecture=x64 /m /t:stout-tests
3rdparty\stout\tests\Debug\stout-tests.exe --gtest_filter="*Json*"
Thanks,
Joseph Wu
Re: Review Request 55435: Windows: Fixed the locale guard in
jsonify.hpp.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55435/#review161365
-----------------------------------------------------------
3rdparty/stout/include/stout/jsonify.hpp (lines 66 - 72)
<https://reviews.apache.org/r/55435/#comment232605>
Add a TODO about pulling the locale manipulation to an eventual `locale.hpp`
3rdparty/stout/include/stout/jsonify.hpp (lines 75 - 78)
<https://reviews.apache.org/r/55435/#comment232604>
We actually should verify it the application already has that enabled and revert only if per thread locale wasn't set up when we create the RAII. e.g.
```c++
per_thread = (_configthreadlocale(_ENABLE_PER_THREAD_LOCALE) == _ENABLE_PER_THREAD_LOCALE);
// play with locale
// ...
if (!per_thread) {
_configthreadlocale(_DISABLE_PER_THREAD_LOCALE)
}
```
- Alexander Rojas
On Jan. 12, 2017, 1:56 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55435/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2017, 1:56 a.m.)
>
>
> Review request for mesos, Alexander Rojas, Daniel Pravat, Alex Clemmer, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changing locales in Windows is mainly accomplished via `setlocale`,
> which takes a bit mask (with constants named slightly differently
> than in the POSIX headers) and a `char*`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/jsonify.hpp 3c48046e087de2a66139a31449327fd94c149371
>
> Diff: https://reviews.apache.org/r/55435/diff/
>
>
> Testing
> -------
>
> Windows:
>
> msbuild Mesos.sln /p:PreferredToolArchitecture=x64 /m /t:stout-tests
> 3rdparty\stout\tests\Debug\stout-tests.exe --gtest_filter="*Json*"
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 55435: Windows: Fixed the locale guard in
jsonify.hpp.
Posted by Michael Park <mp...@apache.org>.
> On Jan. 12, 2017, 1:50 a.m., Mesos ReviewBot wrote:
> > Bad patch!
> >
> > Reviews applied: [55435, 55029, 55028, 55026, 55025, 55024, 55023, 55022]
> >
> > Failed command: python support/apply-reviews.py -n -r 55022
> >
> > Error:
> > Traceback (most recent call last):
> > File "support/apply-reviews.py", line 349, in <module>
> > reviewboard()
> > File "support/apply-reviews.py", line 328, in reviewboard
> > apply_review()
> > File "support/apply-reviews.py", line 121, in apply_review
> > fetch_patch()
> > File "support/apply-reviews.py", line 150, in fetch_patch
> > r = urllib2.urlopen(patch_url(), context=ssl_create_default_context())
> > File "support/apply-reviews.py", line 131, in ssl_create_default_context
> > context = ssl.SSLContext(ssl.PROTOCOL_SSLv23)
> > AttributeError: 'module' object has no attribute 'SSLContext'
> > Error in atexit._run_exitfuncs:
> > Traceback (most recent call last):
> > File "/usr/lib/python2.7/atexit.py", line 24, in _run_exitfuncs
> > func(*targs, **kargs)
> > File "support/apply-reviews.py", line 119, in <lambda>
> > atexit.register(lambda: os.remove('%s.patch' % patch_id()))
> > OSError: [Errno 2] No such file or directory: '55022.patch'
> > Error in sys.exitfunc:
> > Traceback (most recent call last):
> > File "/usr/lib/python2.7/atexit.py", line 24, in _run_exitfuncs
> > func(*targs, **kargs)
> > File "support/apply-reviews.py", line 119, in <lambda>
> > atexit.register(lambda: os.remove('%s.patch' % patch_id()))
> > OSError: [Errno 2] No such file or directory: '55022.patch'
> >
> > Full log: https://builds.apache.org/job/Mesos-Reviewbot/16691/console
Looks like http://stackoverflow.com/questions/28228214/ssl-module-object-has-no-attribute-sslcontext
and I can see that we're getting Python 2.7.6 on that particular box... Maybe we need a more strict
constraint on the ubuntu version or something on Reviewbot...
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55435/#review161363
-----------------------------------------------------------
On Jan. 11, 2017, 4:56 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55435/
> -----------------------------------------------------------
>
> (Updated Jan. 11, 2017, 4:56 p.m.)
>
>
> Review request for mesos, Alexander Rojas, Daniel Pravat, Alex Clemmer, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changing locales in Windows is mainly accomplished via `setlocale`,
> which takes a bit mask (with constants named slightly differently
> than in the POSIX headers) and a `char*`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/jsonify.hpp 3c48046e087de2a66139a31449327fd94c149371
>
> Diff: https://reviews.apache.org/r/55435/diff/
>
>
> Testing
> -------
>
> Windows:
>
> msbuild Mesos.sln /p:PreferredToolArchitecture=x64 /m /t:stout-tests
> 3rdparty\stout\tests\Debug\stout-tests.exe --gtest_filter="*Json*"
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 55435: Windows: Fixed the locale guard in
jsonify.hpp.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55435/#review161363
-----------------------------------------------------------
Bad patch!
Reviews applied: [55435, 55029, 55028, 55026, 55025, 55024, 55023, 55022]
Failed command: python support/apply-reviews.py -n -r 55022
Error:
Traceback (most recent call last):
File "support/apply-reviews.py", line 349, in <module>
reviewboard()
File "support/apply-reviews.py", line 328, in reviewboard
apply_review()
File "support/apply-reviews.py", line 121, in apply_review
fetch_patch()
File "support/apply-reviews.py", line 150, in fetch_patch
r = urllib2.urlopen(patch_url(), context=ssl_create_default_context())
File "support/apply-reviews.py", line 131, in ssl_create_default_context
context = ssl.SSLContext(ssl.PROTOCOL_SSLv23)
AttributeError: 'module' object has no attribute 'SSLContext'
Error in atexit._run_exitfuncs:
Traceback (most recent call last):
File "/usr/lib/python2.7/atexit.py", line 24, in _run_exitfuncs
func(*targs, **kargs)
File "support/apply-reviews.py", line 119, in <lambda>
atexit.register(lambda: os.remove('%s.patch' % patch_id()))
OSError: [Errno 2] No such file or directory: '55022.patch'
Error in sys.exitfunc:
Traceback (most recent call last):
File "/usr/lib/python2.7/atexit.py", line 24, in _run_exitfuncs
func(*targs, **kargs)
File "support/apply-reviews.py", line 119, in <lambda>
atexit.register(lambda: os.remove('%s.patch' % patch_id()))
OSError: [Errno 2] No such file or directory: '55022.patch'
Full log: https://builds.apache.org/job/Mesos-Reviewbot/16691/console
- Mesos ReviewBot
On Jan. 12, 2017, 12:56 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55435/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2017, 12:56 a.m.)
>
>
> Review request for mesos, Alexander Rojas, Daniel Pravat, Alex Clemmer, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changing locales in Windows is mainly accomplished via `setlocale`,
> which takes a bit mask (with constants named slightly differently
> than in the POSIX headers) and a `char*`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/jsonify.hpp 3c48046e087de2a66139a31449327fd94c149371
>
> Diff: https://reviews.apache.org/r/55435/diff/
>
>
> Testing
> -------
>
> Windows:
>
> msbuild Mesos.sln /p:PreferredToolArchitecture=x64 /m /t:stout-tests
> 3rdparty\stout\tests\Debug\stout-tests.exe --gtest_filter="*Json*"
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 55435: Windows: Fixed the locale guard in
jsonify.hpp.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Jan. 11, 2017, 7 p.m., Michael Park wrote:
> > 3rdparty/stout/include/stout/jsonify.hpp, lines 75-78
> > <https://reviews.apache.org/r/55435/diff/2/?file=1603287#file1603287line75>
> >
> > Shouldn't we call `_configthreadlocale(_DISABLE_PER_THREAD_LOCALE);` here to recover the original `setlocale` behavior?
Added comments to explain the proper usage of the locale guard. The destructor now restores the previous config.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55435/#review161347
-----------------------------------------------------------
On Jan. 12, 2017, 3:14 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55435/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2017, 3:14 p.m.)
>
>
> Review request for mesos, Alexander Rojas, Daniel Pravat, Alex Clemmer, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changing locales in Windows is mainly accomplished via `setlocale`,
> which takes a bit mask (with constants named slightly differently
> than in the POSIX headers) and a `char*`.
>
> This also adds a comment to the locale guard.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/jsonify.hpp 3c48046e087de2a66139a31449327fd94c149371
>
> Diff: https://reviews.apache.org/r/55435/diff/
>
>
> Testing
> -------
>
> Windows:
>
> msbuild Mesos.sln /p:PreferredToolArchitecture=x64 /m /t:stout-tests
> 3rdparty\stout\tests\Debug\stout-tests.exe --gtest_filter="*Json*"
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 55435: Windows: Fixed the locale guard in
jsonify.hpp.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55435/#review161347
-----------------------------------------------------------
3rdparty/stout/include/stout/jsonify.hpp (lines 75 - 78)
<https://reviews.apache.org/r/55435/#comment232590>
Shouldn't we call `_configthreadlocale(_DISABLE_PER_THREAD_LOCALE);` here to recover the original `setlocale` behavior?
- Michael Park
On Jan. 11, 2017, 4:56 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55435/
> -----------------------------------------------------------
>
> (Updated Jan. 11, 2017, 4:56 p.m.)
>
>
> Review request for mesos, Alexander Rojas, Daniel Pravat, Alex Clemmer, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changing locales in Windows is mainly accomplished via `setlocale`,
> which takes a bit mask (with constants named slightly differently
> than in the POSIX headers) and a `char*`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/jsonify.hpp 3c48046e087de2a66139a31449327fd94c149371
>
> Diff: https://reviews.apache.org/r/55435/diff/
>
>
> Testing
> -------
>
> Windows:
>
> msbuild Mesos.sln /p:PreferredToolArchitecture=x64 /m /t:stout-tests
> 3rdparty\stout\tests\Debug\stout-tests.exe --gtest_filter="*Json*"
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 55435: Windows: Fixed the locale guard in
jsonify.hpp.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55435/
-----------------------------------------------------------
(Updated Jan. 11, 2017, 4:56 p.m.)
Review request for mesos, Alexander Rojas, Daniel Pravat, Alex Clemmer, and Michael Park.
Changes
-------
Reduced the number of `ifdef`s (3 -> 1)and made the locale guard thread safe.
Repository: mesos
Description
-------
Changing locales in Windows is mainly accomplished via `setlocale`,
which takes a bit mask (with constants named slightly differently
than in the POSIX headers) and a `char*`.
Diffs (updated)
-----
3rdparty/stout/include/stout/jsonify.hpp 3c48046e087de2a66139a31449327fd94c149371
Diff: https://reviews.apache.org/r/55435/diff/
Testing
-------
Windows:
msbuild Mesos.sln /p:PreferredToolArchitecture=x64 /m /t:stout-tests
3rdparty\stout\tests\Debug\stout-tests.exe --gtest_filter="*Json*"
Thanks,
Joseph Wu