Re: RFR: 8201274: Launch Single-File Source-Code Programs

2018-05-04 Thread Jonathan Gibbons
Here's an update to the previously proposed patch for JEP 330: Launch 
Single-File Source-Code Programs.
It includes all review feedback so far. The changes are mostly minor, 
but with the addition of more test cases.


The webrev includes a delta-webrev for those that just want to see what 
has changed since last time.


Full webrev: http://cr.openjdk.java.net/~jjg/8201274/webrev.mq/index.html

Original webrev: 
http://cr.openjdk.java.net/~jjg/8201274/webrev.mq/webrev.v1/index.html
Delta webrev: 
http://cr.openjdk.java.net/~jjg/8201274/webrev.mq/webrev.v2/index.html


Note that the work is temporarily blocked by JDK-8202387: javac 
--release 11 not supported.
A fix for that is underway and in review: 
http://mail.openjdk.java.net/pipermail/compiler-dev/2018-May/011868.html
This work has been tested using a workaround for this issue, and will be 
tested again when the real fix is in place.


-- Jon

On 04/12/2018 01:15 PM, Jonathan Gibbons wrote:

Please review an initial implementation for the feature described in
JEP 330: Launch Single-File Source-Code Programs.

The work is described in the JEP and CSR, and falls into various parts:

  * The part to handle the new command-line options is in the native
Java launcher code.
  * The part to invoke the compiler and subsequently execute the code
found in the source file is in a new class in the jdk.compiler module.
  * There are some minor Makefile changes, to add support for a new
resource file.

There are no changes to javac itself.

JEP: http://openjdk.java.net/jeps/330
JBS: https://bugs.openjdk.java.net/browse/JDK-8201274
CSR: https://bugs.openjdk.java.net/browse/JDK-8201275
Webrev: http://cr.openjdk.java.net/~jjg/8201274/webrev.00/

-- Jon




Re: RFR : JDK-8202387: javac --release 11 not supported

2018-05-04 Thread Jonathan Gibbons

I like the Makefile improvement.

+1

-- Jon

On 05/04/2018 06:48 AM, Jan Lahoda wrote:

[+build-dev]

On 4.5.2018 03:27, Jonathan Gibbons wrote:

OK.

It would be even better, perhaps in a subsequent update, if
make/gendata/Gendata-jdk.compiler.gmk
did not have to be updated for each release ... i.e. by changing A to B,
and soon to C etc. The version
letter can surely be inferred by the system.


True. Updated webrevs:
http://cr.openjdk.java.net/~jlahoda/8202387/code.01/
(code changes, including update to Makefiles to automatically infer 
the current JDK version)


http://cr.openjdk.java.net/~jlahoda/8202387/data.01/
(historical data for JDK 10, similar patches will be needed for each 
new JDK version)


How does this look?

Thanks,
Jan



-- Jon



On 05/03/2018 11:07 AM, Jan Lahoda wrote:

Hi,

Bug:
https://bugs.openjdk.java.net/browse/JDK-8202387

This patch adds historical data for JDK 10 and adds support for
--release 11.

To simplify adding new platforms, the CreateSymbols tool is updated to
support incrementally adding platform support. So now it is possible
to run command like:
/bin/java 
build.tools.symbolgenerator.CreateSymbols
build-description-incremental symbols include.list

to add historical data for JDK 10. In the future it might even be
possible to use the source launcher so that one would not need to
compile the tool before use.

The webrevs are split into two:
-updating the CreateSymbols tool, and adding a test that verifies that
"--release " works (as suggested):
http://cr.openjdk.java.net/~jlahoda/8202387/code.00/
-actually adding the data for JDK 10, and adding --release 11:
http://cr.openjdk.java.net/~jlahoda/8202387/data.00/

How does this look?

Thanks,
Jan






Re: RFR: JDK-8202319: Fix compilation warnings in Solaris debug builds for DevStudio 12.6

2018-05-04 Thread Kim Barrett
Patch looks good.  I will sponsor.



Re: [OpenJDK 2D-Dev] [11] RFR for JDK-8202476: ImageLib is broken in 32 bit Windows

2018-05-04 Thread Alexey Ivanov

Thank you!

--
Alexey

On 04/05/2018 18:53, Phil Race wrote:

Yes, your confirmation on the testing was all that was needed.

-phil.

On 5/4/2018 10:45 AM, Alexey Ivanov wrote:

Hi Phil,

Just to confirm: do you approve the change?

Thank you,
Alexey

On 02/05/2018 19:24, Alexey Ivanov wrote:

Hi Phil,

Thank you for your review.

On 02/05/2018 17:28, Phil Race wrote:
So ... the original change that removed the mapfiles broke the 32 
bit build
because of inconsistency between declarations + definitions of some 
functions.

It did not affect 64 bit build because JNICALL is a no-op there.

The next change (8201226) added JNICALL to make it consistent, but
was not reviewed on 2d and was pushed without such review
and may have fixed build issues but did NOT fix the product.


Yes, you're right. It made the product buildable but it did not fix 
the product.




This change now removes JNICALL where it is not needed and fixes 
the product and does not break

any builds!

Please confirm that you've run 64+32 bit builds through the build 
system AND run imaging tests

on both of those.


Yes, I ran 64 builds through the build system and ran 32 bit Windows 
build on my laptop.


I ran regression tests from test/jdk/java/awt/image with both 64 and 
32 bit builds on Windows. No failures.



Regards,
Alexey



After that is confirmed you have my OK.

-phil.

On 05/02/2018 05:03 AM, Magnus Ihse Bursie wrote:
Looks good to me, but you should have a reviewer from the client 
team as well.


/Magnus


2 maj 2018 kl. 11:52 skrev Alexey Ivanov :

Hi,

Could you please review the following fix for jdk11?

bug: https://bugs.openjdk.java.net/browse/JDK-8202476
webrev: http://cr.openjdk.java.net/~aivanov/8202476/jdk11/webrev.0/

This is a follow-up fix for JDK-8201226 which enabled building 
JDK for 32 bit Windows, its code review: 
http://mail.openjdk.java.net/pipermail/build-dev/2018-April/021553.html 



I found this issue:
http://mail.openjdk.java.net/pipermail/2d-dev/2018-April/009150.html


This fix removes JNICALL modifiers from exported functions in 
medialib.

After the fix, the failing tests in test/jdk/java/awt/image pass.


Thank you in advance.

Regards,
Alexey












Re: [OpenJDK 2D-Dev] [11] RFR for JDK-8202476: ImageLib is broken in 32 bit Windows

2018-05-04 Thread Phil Race

Yes, your confirmation on the testing was all that was needed.

-phil.

On 5/4/2018 10:45 AM, Alexey Ivanov wrote:

Hi Phil,

Just to confirm: do you approve the change?

Thank you,
Alexey

On 02/05/2018 19:24, Alexey Ivanov wrote:

Hi Phil,

Thank you for your review.

On 02/05/2018 17:28, Phil Race wrote:
So ... the original change that removed the mapfiles broke the 32 
bit build
because of inconsistency between declarations + definitions of some 
functions.

It did not affect 64 bit build because JNICALL is a no-op there.

The next change (8201226) added JNICALL to make it consistent, but
was not reviewed on 2d and was pushed without such review
and may have fixed build issues but did NOT fix the product.


Yes, you're right. It made the product buildable but it did not fix 
the product.




This change now removes JNICALL where it is not needed and fixes the 
product and does not break

any builds!

Please confirm that you've run 64+32 bit builds through the build 
system AND run imaging tests

on both of those.


Yes, I ran 64 builds through the build system and ran 32 bit Windows 
build on my laptop.


I ran regression tests from test/jdk/java/awt/image with both 64 and 
32 bit builds on Windows. No failures.



Regards,
Alexey



After that is confirmed you have my OK.

-phil.

On 05/02/2018 05:03 AM, Magnus Ihse Bursie wrote:
Looks good to me, but you should have a reviewer from the client 
team as well.


/Magnus


2 maj 2018 kl. 11:52 skrev Alexey Ivanov :

Hi,

Could you please review the following fix for jdk11?

bug: https://bugs.openjdk.java.net/browse/JDK-8202476
webrev: http://cr.openjdk.java.net/~aivanov/8202476/jdk11/webrev.0/

This is a follow-up fix for JDK-8201226 which enabled building JDK 
for 32 bit Windows, its code review: 
http://mail.openjdk.java.net/pipermail/build-dev/2018-April/021553.html 



I found this issue:
http://mail.openjdk.java.net/pipermail/2d-dev/2018-April/009150.html


This fix removes JNICALL modifiers from exported functions in 
medialib.

After the fix, the failing tests in test/jdk/java/awt/image pass.


Thank you in advance.

Regards,
Alexey










Re: [OpenJDK 2D-Dev] [11] RFR for JDK-8202476: ImageLib is broken in 32 bit Windows

2018-05-04 Thread Alexey Ivanov

Hi Phil,

Just to confirm: do you approve the change?

Thank you,
Alexey

On 02/05/2018 19:24, Alexey Ivanov wrote:

Hi Phil,

Thank you for your review.

On 02/05/2018 17:28, Phil Race wrote:
So ... the original change that removed the mapfiles broke the 32 bit 
build
because of inconsistency between declarations + definitions of some 
functions.

It did not affect 64 bit build because JNICALL is a no-op there.

The next change (8201226) added JNICALL to make it consistent, but
was not reviewed on 2d and was pushed without such review
and may have fixed build issues but did NOT fix the product.


Yes, you're right. It made the product buildable but it did not fix 
the product.




This change now removes JNICALL where it is not needed and fixes the 
product and does not break

any builds!

Please confirm that you've run 64+32 bit builds through the build 
system AND run imaging tests

on both of those.


Yes, I ran 64 builds through the build system and ran 32 bit Windows 
build on my laptop.


I ran regression tests from test/jdk/java/awt/image with both 64 and 
32 bit builds on Windows. No failures.



Regards,
Alexey



After that is confirmed you have my OK.

-phil.

On 05/02/2018 05:03 AM, Magnus Ihse Bursie wrote:
Looks good to me, but you should have a reviewer from the client 
team as well.


/Magnus


2 maj 2018 kl. 11:52 skrev Alexey Ivanov :

Hi,

Could you please review the following fix for jdk11?

bug: https://bugs.openjdk.java.net/browse/JDK-8202476
webrev: http://cr.openjdk.java.net/~aivanov/8202476/jdk11/webrev.0/

This is a follow-up fix for JDK-8201226 which enabled building JDK 
for 32 bit Windows, its code review: 
http://mail.openjdk.java.net/pipermail/build-dev/2018-April/021553.html 



I found this issue:
http://mail.openjdk.java.net/pipermail/2d-dev/2018-April/009150.html


This fix removes JNICALL modifiers from exported functions in 
medialib.

After the fix, the failing tests in test/jdk/java/awt/image pass.


Thank you in advance.

Regards,
Alexey








Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

2018-05-04 Thread Kim Barrett
> On May 4, 2018, at 11:42 AM, Alan Bateman  wrote:
> 
> On 04/05/2018 16:31, B. Blaser wrote:
>> :
>> 
>> I'll create a new JBS issue (unless you want to) since the fix is
>> mostly located in core-libs and only intended to make the build
>> complete.
>> But I'll still need someone to review and push the fix, would you be
>> interested in doing this?
>> 
> I think someone needs to explore the behavior on other platforms too as the 
> #ifndef __linux__ patches are ugly. Is there discussion on the original 
> thread about this?
> 
> -Alan

Yes, see https://bugs.openjdk.java.net/browse/JDK-8202353



Re: RFR : JDK-8202387: javac --release 11 not supported

2018-05-04 Thread Remi Forax
Hi Jan,
there is several occurences of Arrays.asList() that can be replaced by 
List.of() to make them really immutable.

in CreateSymbols.java
  in dumpCurrentClasses,
 while ((read = in.read()) != (-1)) {
   baos.write(read);
 }
   should be replaced by
 in.transferTo(baos);

in TransitiveDependencies.java,
  - todo should be an ArrayDeque instead of a LinkedList, array based data 
structure are usually faster
  - newBufferedWriter can takes only one argument

in PreviewOptionTest.java,
  - versionsToTest.stream().forEach can be replaced by versionsToTest.forEach

regards,
Rémi

- Mail original -
> De: "jan lahoda" 
> À: "jonathan gibbons" , "compiler-dev" 
> ,
> build-dev@openjdk.java.net
> Envoyé: Vendredi 4 Mai 2018 15:48:55
> Objet: Re: RFR : JDK-8202387: javac --release 11 not supported

> [+build-dev]
> 
> On 4.5.2018 03:27, Jonathan Gibbons wrote:
>> OK.
>>
>> It would be even better, perhaps in a subsequent update, if
>> make/gendata/Gendata-jdk.compiler.gmk
>> did not have to be updated for each release ... i.e. by changing A to B,
>> and soon to C etc. The version
>> letter can surely be inferred by the system.
> 
> True. Updated webrevs:
> http://cr.openjdk.java.net/~jlahoda/8202387/code.01/
> (code changes, including update to Makefiles to automatically infer the
> current JDK version)
> 
> http://cr.openjdk.java.net/~jlahoda/8202387/data.01/
> (historical data for JDK 10, similar patches will be needed for each new
> JDK version)
> 
> How does this look?
> 
> Thanks,
> Jan
> 
>>
>> -- Jon
>>
>>
>>
>> On 05/03/2018 11:07 AM, Jan Lahoda wrote:
>>> Hi,
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8202387
>>>
>>> This patch adds historical data for JDK 10 and adds support for
>>> --release 11.
>>>
>>> To simplify adding new platforms, the CreateSymbols tool is updated to
>>> support incrementally adding platform support. So now it is possible
>>> to run command like:
>>> /bin/java 
>>> build.tools.symbolgenerator.CreateSymbols
>>> build-description-incremental symbols include.list
>>>
>>> to add historical data for JDK 10. In the future it might even be
>>> possible to use the source launcher so that one would not need to
>>> compile the tool before use.
>>>
>>> The webrevs are split into two:
>>> -updating the CreateSymbols tool, and adding a test that verifies that
>>> "--release " works (as suggested):
>>> http://cr.openjdk.java.net/~jlahoda/8202387/code.00/
>>> -actually adding the data for JDK 10, and adding --release 11:
>>> http://cr.openjdk.java.net/~jlahoda/8202387/data.00/
>>>
>>> How does this look?
>>>
>>> Thanks,
>>> Jan


Re: RFR: 8201429: Support AIX Input Method Editor (IME) for AWT Input Method Framework (IMF)

2018-05-04 Thread Erik Joelsson

Hello,

It looks like what you are trying to achieve is to override 
awt_InputMethod.c with an OS specific version of that file. We have a 
construct for this in SetupNativeCompilation that should handle it 
automatically, if you just list the source dirs in priority order. I 
would suggest leveraging this by making this change instead:


First in the list of LIBAWT_XAWT_DIRS (line 272), prepend a line like this:

$(wildcard 
$(TOPDIR)/src/java.desktop/$(OPENJDK_TARGET_OS)/native/libawt_xawt) \


/Erik


On 2018-05-04 07:07, Langer, Christoph wrote:

Hi,

please help reviewing the contribution of the support for the AIX Input Method 
Editor (IME) in AWT's Input Method Framework.

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8201429.1/
Bug: https://bugs.openjdk.java.net/browse/JDK-8201429

I took Ichiroh's initial proposal [1] and tried to integrate it better with 
existing code. I have split 
src/java.desktop/unix/classes/sun/awt/X11InputMethod.java into
a) a base class containing the common code: 
src/java.desktop/unix/classes/sun/awt/X11InputMethodBase.java
b) a specific class for the common Linux/Unixes: 
src/java.desktop/unix/classes/sun/awt/X11InputMethod.java and
c) a specific class for AIX: 
src/java.desktop/aix/classes/sun/awt/X11InputMethod.java

The AIX specific additions to the native code of 
src/java.desktop/unix/native/libawt_xawt/awt/awt_InputMethod.c were so much 
that I decided to add a specific implementation file for AIX: 
src/java.desktop/aix/native/libawt_xawt/awt/awt_InputMethod_aix.c. The changes 
to the former file are some cleanups.

I'm still in the process of testing the changes - but maybe you can run further 
tests, especially on non-AIX unixes to make sure we didn't break something.

Thanks & Best regards
Christoph

[1]: http://mail.openjdk.java.net/pipermail/awt-dev/2018-April/013869.html





Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

2018-05-04 Thread Alan Bateman

On 04/05/2018 16:31, B. Blaser wrote:

:

I'll create a new JBS issue (unless you want to) since the fix is
mostly located in core-libs and only intended to make the build
complete.
But I'll still need someone to review and push the fix, would you be
interested in doing this?

I think someone needs to explore the behavior on other platforms too as 
the #ifndef __linux__ patches are ugly. Is there discussion on the 
original thread about this?


-Alan


Re: RFR : JDK-8202387: javac --release 11 not supported

2018-05-04 Thread Erik Joelsson

Build change looks good.

/Erik


On 2018-05-04 06:48, Jan Lahoda wrote:

[+build-dev]

On 4.5.2018 03:27, Jonathan Gibbons wrote:

OK.

It would be even better, perhaps in a subsequent update, if
make/gendata/Gendata-jdk.compiler.gmk
did not have to be updated for each release ... i.e. by changing A to B,
and soon to C etc. The version
letter can surely be inferred by the system.


True. Updated webrevs:
http://cr.openjdk.java.net/~jlahoda/8202387/code.01/
(code changes, including update to Makefiles to automatically infer 
the current JDK version)


http://cr.openjdk.java.net/~jlahoda/8202387/data.01/
(historical data for JDK 10, similar patches will be needed for each 
new JDK version)


How does this look?

Thanks,
    Jan



-- Jon



On 05/03/2018 11:07 AM, Jan Lahoda wrote:

Hi,

Bug:
https://bugs.openjdk.java.net/browse/JDK-8202387

This patch adds historical data for JDK 10 and adds support for
--release 11.

To simplify adding new platforms, the CreateSymbols tool is updated to
support incrementally adding platform support. So now it is possible
to run command like:
/bin/java 
build.tools.symbolgenerator.CreateSymbols
build-description-incremental symbols include.list

to add historical data for JDK 10. In the future it might even be
possible to use the source launcher so that one would not need to
compile the tool before use.

The webrevs are split into two:
-updating the CreateSymbols tool, and adding a test that verifies that
"--release " works (as suggested):
http://cr.openjdk.java.net/~jlahoda/8202387/code.00/
-actually adding the data for JDK 10, and adding --release 11:
http://cr.openjdk.java.net/~jlahoda/8202387/data.00/

How does this look?

Thanks,
    Jan






Re: RFR(L) : 8199382 : [TESTBUG] Open source VM testbase JDI tests

2018-05-04 Thread Erik Joelsson

Build change looks good.

/Erik


On 2018-05-03 21:14, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev/8199382/webrev.00/index.html

577169 lines changed: 577169 ins; 0 del; 0 mod;

Hi all,

could you please review the patch which open sources JDI tests from vm 
testbase? These tests cover different aspects of JDI implementation.

As usually w/ VM testbase code, these tests are old, they have been run in 
hotspot testing for a long period of time. Originally, these tests were run by 
a test harness different from jtreg and had different build and execution 
schemes, some parts couldn't be easily translated to jtreg, so tests might have 
actions or pieces of code which look weird. In a long term, we are planning to 
rework them.

JBS: https://bugs.openjdk.java.net/browse/JDK-8199382
webrev:  http://cr.openjdk.java.net/~iignatyev/8199382/webrev.00/index.html
testing: vmTestbase_nsk_jdi test group

Thanks,
-- Igor




Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

2018-05-04 Thread B. Blaser
On 3 May 2018 at 21:43, Kim Barrett  wrote:
>> On May 3, 2018, at 1:52 PM, B. Blaser  wrote:
>>
>> Hi,
>>
>> On 2 May 2018 at 22:40, Kim Barrett  wrote:
 On May 2, 2018, at 5:10 AM, Michal Vala  wrote:



 On 05/01/2018 07:59 PM, Kim Barrett wrote:
>> On Apr 27, 2018, at 4:26 PM, Michal Vala  wrote:
>> Someone to sponsor this please?
> Do you have a sponsor yet?  If not, I’ll do it.

 No, I don't. I'd really appreciate if you sponsor it.
>>>
>>> Will do.  I should be able to take care of it in the next day or so.
>>>
>>
>> I've noted some similar issues at several other places which makes the
>> JDK build fail on Linux with glibc = 2.26, see comments here:
>> https://bugs.openjdk.java.net/browse/JDK-8202353
>>
>> Here is a patch for that, does this look reasonable (tier1 seems OK)?
>
> I think we should move in the direction of eliminating the use of readdir_r 
> entirely,
> either under JDK-8202353, or leave that as only the HotSpot part and have 
> another
> RFE for the uses in java.base/unix/native.

I'll create a new JBS issue (unless you want to) since the fix is
mostly located in core-libs and only intended to make the build
complete.
But I'll still need someone to review and push the fix, would you be
interested in doing this?

Thanks,
Bernard


RFR: 8201429: Support AIX Input Method Editor (IME) for AWT Input Method Framework (IMF)

2018-05-04 Thread Langer, Christoph
Hi,

please help reviewing the contribution of the support for the AIX Input Method 
Editor (IME) in AWT's Input Method Framework.

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8201429.1/
Bug: https://bugs.openjdk.java.net/browse/JDK-8201429

I took Ichiroh's initial proposal [1] and tried to integrate it better with 
existing code. I have split 
src/java.desktop/unix/classes/sun/awt/X11InputMethod.java into
a) a base class containing the common code: 
src/java.desktop/unix/classes/sun/awt/X11InputMethodBase.java
b) a specific class for the common Linux/Unixes: 
src/java.desktop/unix/classes/sun/awt/X11InputMethod.java and
c) a specific class for AIX: 
src/java.desktop/aix/classes/sun/awt/X11InputMethod.java

The AIX specific additions to the native code of 
src/java.desktop/unix/native/libawt_xawt/awt/awt_InputMethod.c were so much 
that I decided to add a specific implementation file for AIX: 
src/java.desktop/aix/native/libawt_xawt/awt/awt_InputMethod_aix.c. The changes 
to the former file are some cleanups.

I'm still in the process of testing the changes - but maybe you can run further 
tests, especially on non-AIX unixes to make sure we didn't break something.

Thanks & Best regards
Christoph

[1]: http://mail.openjdk.java.net/pipermail/awt-dev/2018-April/013869.html



Re: RFR : JDK-8202387: javac --release 11 not supported

2018-05-04 Thread Jan Lahoda

[+build-dev]

On 4.5.2018 03:27, Jonathan Gibbons wrote:

OK.

It would be even better, perhaps in a subsequent update, if
make/gendata/Gendata-jdk.compiler.gmk
did not have to be updated for each release ... i.e. by changing A to B,
and soon to C etc. The version
letter can surely be inferred by the system.


True. Updated webrevs:
http://cr.openjdk.java.net/~jlahoda/8202387/code.01/
(code changes, including update to Makefiles to automatically infer the 
current JDK version)


http://cr.openjdk.java.net/~jlahoda/8202387/data.01/
(historical data for JDK 10, similar patches will be needed for each new 
JDK version)


How does this look?

Thanks,
Jan



-- Jon



On 05/03/2018 11:07 AM, Jan Lahoda wrote:

Hi,

Bug:
https://bugs.openjdk.java.net/browse/JDK-8202387

This patch adds historical data for JDK 10 and adds support for
--release 11.

To simplify adding new platforms, the CreateSymbols tool is updated to
support incrementally adding platform support. So now it is possible
to run command like:
/bin/java 
build.tools.symbolgenerator.CreateSymbols
build-description-incremental symbols include.list

to add historical data for JDK 10. In the future it might even be
possible to use the source launcher so that one would not need to
compile the tool before use.

The webrevs are split into two:
-updating the CreateSymbols tool, and adding a test that verifies that
"--release " works (as suggested):
http://cr.openjdk.java.net/~jlahoda/8202387/code.00/
-actually adding the data for JDK 10, and adding --release 11:
http://cr.openjdk.java.net/~jlahoda/8202387/data.00/

How does this look?

Thanks,
Jan




Re: Windows: problem with msvxxx.dll locations containing spaces

2018-05-04 Thread Thomas Stüfe
Hi Erik,

that worked on both machines for all builds.

Thanks, Thomas

On Thu, May 3, 2018 at 10:13 PM, Erik Joelsson  wrote:
> Hello,
>
> In my case, VCINSTALLDIR is in short form already. Could you try changing
> line 543 from BASIC_WINDOWS_REWRITE_AS_UNIX_PATH to BASIC_FIXUP_PATH?
>
> /Erik
>
>
>
> On 2018-05-03 11:17, Thomas Stüfe wrote:
>>
>> Hi Erik,
>>
>> I see the error on two very different machines. One is my private
>> machine - windows 7, VS2013 and VS2017 installed. The second one is my
>> corporate Laptop, Windows 10 and only VS2013.
>>
>> In both cases I use a very recent cygwin64 installation.
>>
>> Both cases run in TOOLCHAIN_SETUP_MSVC_DLL into the "# Probe: Using
>> well-known location from Visual Studio 12.0 and older" case, and in
>> the for loop at line 559 attempt to tokenize the POSSIBLE_MSVC_DLL
>> variable content. Here the 64bit build on my corporate laptop:
>>
>> checking if fixpath.exe works... yes
>> POSSIBLE_MSVC_DLL /cygdrive/c/Program
>> POSSIBLE_MSVC_DLL Files
>> POSSIBLE_MSVC_DLL (x86)/Microsoft
>> POSSIBLE_MSVC_DLL Visual
>> POSSIBLE_MSVC_DLL Studio
>> POSSIBLE_MSVC_DLL 12.0/VC/redist/x64/Microsoft.VC120.CRT/msvcr120.dll
>> configure: Found msvcr120.dll at
>> /cygdrive/c/WINDOWS/system32/msvcr120.dll using well-known location in
>> SYSTEMROOT
>> checking found msvcr120.dll architecture... ok
>> checking for msvcr120.dll... /cygdrive/c/WINDOWS/system32/msvcr120.dll
>> POSSIBLE_MSVC_DLL /cygdrive/c/Program
>> POSSIBLE_MSVC_DLL Files
>> POSSIBLE_MSVC_DLL (x86)/Microsoft
>> POSSIBLE_MSVC_DLL Visual
>> POSSIBLE_MSVC_DLL Studio
>> POSSIBLE_MSVC_DLL 12.0/VC/redist/x64/Microsoft.VC120.CRT/msvcp120.dll
>> configure: Found msvcp120.dll at
>> /cygdrive/c/WINDOWS/system32/msvcp120.dll using well-known location in
>> SYSTEMROOT
>>
>> As you can see, we fall back to the default in windows/system32, which
>> happens to be working for 64bit. On 32bit, we get this error:
>>
>> checking if fixpath.exe works... yes
>> POSSIBLE_MSVC_DLL /cygdrive/c/Program
>> POSSIBLE_MSVC_DLL Files
>> POSSIBLE_MSVC_DLL (x86)/Microsoft
>> POSSIBLE_MSVC_DLL Visual
>> POSSIBLE_MSVC_DLL Studio
>> POSSIBLE_MSVC_DLL 12.0/VC/redist/x86/Microsoft.VC120.CRT/msvcr120.dll
>> configure: Found msvcr120.dll at
>> /cygdrive/c/WINDOWS/system32/msvcr120.dll using well-known location in
>> SYSTEMROOT
>> checking found msvcr120.dll architecture... incorrect, ignoring
>> configure: The file type of the located msvcr120.dll is PE32+
>> executable (DLL) (GUI) x86-64, for MS Windows
>> configure: Found msvcr120.dll at /cygdrive/c/Program Files
>> (x86)/Microsoft Visual Studio
>> 12.0/VC/redist/arm/Microsoft.VC120.CRT/msvcr120.dll using search of
>> VCINSTALLDIR
>> checking found msvcr120.dll architecture... incorrect, ignoring
>> configure: The file type of the located msvcr120.dll is PE32
>> executable (DLL) (GUI) ARMv7 Thumb, for MS Windows
>> checking for msvcr120.dll... no
>> configure: error: Could not find msvcr120.dll. Please specify using
>> --with-msvcr-dll.
>> configure exiting with result code 1
>>
>> Best Regards, Thomas
>>
>> On Thu, May 3, 2018 at 7:18 PM, Erik Joelsson 
>> wrote:
>>>
>>> Also, on my windows system, if I try using my local Visual Studio, both
>>> MSVC
>>> dll's are found in the Visual Studio dir, and the spaces are all cleaned
>>> up.
>>> I wonder what's different about your setup, Thomas.
>>>
>>> /Erik
>>>
>>>
>>>
>>> On 2018-05-03 10:13, Erik Joelsson wrote:

 Hello,

 On 2018-05-03 03:41, Magnus Ihse Bursie wrote:
>
> I think the main issue here is that BASIC_FIXUP_PATH should be called
> for
> the located msvcr*.dll files. I don't have time to look at it now, but
> see
> if you can do a rewrite using BASIC_FIXUP_PATH when the potential file
> is
> located. This should remove all spaces from the path.
>
 No, that is not a good solution. I intentionally removed that
 BASIC_FIXUP_PATH because in VS2017, one of the files has a file name
 longer
 than 8 characters and BASIC_FIXUP_PATH rewrites that filename to short
 style. This in turn has weird consequences in make when we copy that
 file
 using generated copy rules. The target file then gets the short style
 name
 literally.

 When I made that change, I looked at all the calls for potential
 locations
 and thought all of them had the directory prepared properly already. It
 seems I was wrong. I think the correct solution is to either get rid of
 spaces earlier for all the input directories we search, or maybe tweak
 BASIC_FIXUP_PATH to not touch the actual filename.

 /Erik
>
> /Magnus
>
> On 2018-05-02 20:46, Thomas Stüfe wrote:
>>
>> Hi,
>>
>> My 32bit builds on Windows were failing since quite a while and I
>> finally had some minutes to look into that.
>>
>> See prior discussion here:
>>
>>