Improve implementation of executing command #258
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…a execution and show the command executed
|
In principle I am all for simplifying the command execution. However, this is a deep change which needs to be tested on several platforms. It makes me nervous to merge it without some community validation first. Ideally we would have all the integration tests passing on all of the major platforms, so that we can verify that they all still work after this change. Unfortunately, this is not the current reality—some integration tests fail on some platforms, for reasons no one has had time to investigate. May I ask why you felt this change is necessary? Does it fix specific problems you encountered? If so, would you be willing to write a new test which fails under the old execution method, but passes with this one? |
|
Hi,
I actually did here 2 things:
1) Replaced the ANT implementation- which I am not sure how it is
implemented with the newest implementation (Newest- from Java 1.5)
Since the NAR compilation is now done with Java 1.7, and Java 1.7 update 21
(If I am not mistaken) had modified the old way the Java used of
Runtime.exec, I prefered to be sure we have the recommendded way of
activating the commands (If the compilation was still with Java 1.5 or 1.6
it was less urgent).
2) Added to the log the command that is being executed, so in case of
failure we will have the full command already in the logs.
I can not remember now if there was a failure or not in any command with
the old implementation, but I did test the new implementaion on 4 platforms
and it works-
- Windows 64 bits with MSVC
- Linux 64 bits with g++
- AIX 64 bits with g++
- Solaris 64 bits with g++
- AIX 64 bith with XLC_r will be tested in few weeks.
Regards
Eyal
…On Tue, Apr 11, 2017 at 7:31 PM, Curtis Rueden ***@***.***> wrote:
In principle I am all for simplifying the command execution. However, this
is a deep change which needs to be tested on several platforms. It makes me
nervous to merge it without some community validation first.
Ideally we would have all the integration tests passing on all of the
major platforms, so that we can verify that they all still work after this
change. Unfortunately, this is not the current reality—some integration
tests fail on some platforms, for reasons no one has had time to
investigate.
May I ask why you felt this change is necessary? Does it fix specific
problems you encountered? If so, would you be willing to write a new test
which fails under the old execution method, but passes with this one?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#258 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AKT4ITJejPeBF7wMElrCQE6uLzPx3DeIks5ru6rkgaJpZM4LqcGA>
.
|
|
Regarding-
Ideally we would have all the integration tests passing on all of the major
platforms, so that we can verify that they all still work after this
change. Unfortunately, this is not the current reality—some integration
tests fail on some platforms, for reasons no one has had time to
investigate.
Can you tell me-
1) How do you run today the integration tests?
2) What tests fails and on which platforms?
3) Can I re-produce the failures?
I am not promising, but I can try to help here.
Eyal
…On Wed, Apr 12, 2017 at 7:33 AM, Eyal Goren ***@***.***> wrote:
Hi,
I actually did here 2 things:
1) Replaced the ANT implementation- which I am not sure how it is
implemented with the newest implementation (Newest- from Java 1.5)
Since the NAR compilation is now done with Java 1.7, and Java 1.7 update
21 (If I am not mistaken) had modified the old way the Java used of
Runtime.exec, I prefered to be sure we have the recommendded way of
activating the commands (If the compilation was still with Java 1.5 or 1.6
it was less urgent).
2) Added to the log the command that is being executed, so in case of
failure we will have the full command already in the logs.
I can not remember now if there was a failure or not in any command with
the old implementation, but I did test the new implementaion on 4 platforms
and it works-
- Windows 64 bits with MSVC
- Linux 64 bits with g++
- AIX 64 bits with g++
- Solaris 64 bits with g++
- AIX 64 bith with XLC_r will be tested in few weeks.
Regards
Eyal
On Tue, Apr 11, 2017 at 7:31 PM, Curtis Rueden ***@***.***>
wrote:
> In principle I am all for simplifying the command execution. However,
> this is a deep change which needs to be tested on several platforms. It
> makes me nervous to merge it without some community validation first.
>
> Ideally we would have all the integration tests passing on all of the
> major platforms, so that we can verify that they all still work after this
> change. Unfortunately, this is not the current reality—some integration
> tests fail on some platforms, for reasons no one has had time to
> investigate.
>
> May I ask why you felt this change is necessary? Does it fix specific
> problems you encountered? If so, would you be willing to write a new test
> which fails under the old execution method, but passes with this one?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#258 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AKT4ITJejPeBF7wMElrCQE6uLzPx3DeIks5ru6rkgaJpZM4LqcGA>
> .
>
|
https://github.com/maven-nar/nar-maven-plugin/wiki/Testing
On Windows, there is an issue: #223. A bit out of date now, though. Haven't tested lately. On macOS, there have been issues in the past which were fixed (see #41), but now there are failures again, at least on my system. I filed new issue #267 documenting which ones.
I dunno! Try
I completely understand. No worries. Any progress you make is appreciated, but I have no expectations. |
|
Can I get example of the failures on both Platforms- Windows and MacOS?
I mean- the exact errors that you get on your environment?
Eyal
…On Mon, Apr 17, 2017 at 8:35 PM, Curtis Rueden ***@***.***> wrote:
How do you run today the integration tests?
https://github.com/maven-nar/nar-maven-plugin/wiki/Testing
1. What tests fails and on which platforms?
On Windows, there is an issue: #223
<#223>. A bit out of
date now, though. Haven't tested lately.
On macOS, there have been issues in the past which were fixed (see #41
<#41>), but now there
are failures again, at least on my system. I filed new issue #267
<#267> documenting
which ones.
1. Can I re-produce the failures?
I dunno! Try mvn -Prun-its and see what happens on your box! 😄
I am not promising, but I can try to help here.
I completely understand. No worries. Any progress you make is appreciated,
but I have no expectations.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#258 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AKT4IRCJvaUqA99v2jcKsOCQn-LNsTNYks5rw6LTgaJpZM4LqcGA>
.
|
|
I am running the Integration tests on my machine (WIndows 10, Java 1.7 and
3 different VS installed) and it pass.
Attaching log.
What am I not doing correct?
I will try to run it also on Mac in the next few days.
Eyal
…On Wed, Apr 19, 2017 at 9:29 PM, Eyal Goren ***@***.***> wrote:
Can I get example of the failures on both Platforms- Windows and MacOS?
I mean- the exact errors that you get on your environment?
Eyal
On Mon, Apr 17, 2017 at 8:35 PM, Curtis Rueden ***@***.***>
wrote:
> How do you run today the integration tests?
>
> https://github.com/maven-nar/nar-maven-plugin/wiki/Testing
>
>
> 1. What tests fails and on which platforms?
>
> On Windows, there is an issue: #223
> <#223>. A bit out of
> date now, though. Haven't tested lately.
>
> On macOS, there have been issues in the past which were fixed (see #41
> <#41>), but now
> there are failures again, at least on my system. I filed new issue #267
> <#267> documenting
> which ones.
>
>
> 1. Can I re-produce the failures?
>
> I dunno! Try mvn -Prun-its and see what happens on your box! 😄
>
> I am not promising, but I can try to help here.
>
> I completely understand. No worries. Any progress you make is
> appreciated, but I have no expectations.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#258 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AKT4IRCJvaUqA99v2jcKsOCQn-LNsTNYks5rw6LTgaJpZM4LqcGA>
> .
>
|
Probably, you are doing everything correctly. It's just that there is a lot of variation between environments. Our old Windows node which was failing ran Windows 7, likely with different version(s) of Visual Studio. It is a lot of work to test a wide variety of configurations. |
|
Can you attach me for the first failure- The target folder? so I can see
what is causing the error?
I already wrote you in the Mac OS issue that I found the source of the
problem- Next week I will try to see if my proposed solution is working.
Eyal
בתאריך 20 באפר׳ 2017 21:35, "Curtis Rueden" <[email protected]> כתב:
… What am I not doing correct?
Probably, you are doing everything correctly. It's just that there is a
lot of variation between environments. Our old Windows node which was
failing ran Windows 7, likely with different version(s) of Visual Studio.
It is a lot of work to test a wide variety of configurations.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#258 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AKT4IbWvKqZD67pMDV3j2DVVG_ztO481ks5rx6V-gaJpZM4LqcGA>
.
|
|
I pushed all IT logs from that attempt to the same gist. Here is |
|
Thanks Curtis,
However- this are the results on the Mac machine.
I already re-produce it and I believe that the problem is that the Mac now
does not pass the DYLD_LIBRARY_PATH to the surefire plugin process, so it
does not find the jnilib.
I suhhest to replace the NarSystem implementation- instead of calling
System.LoadLibrary with the module name (Which depends on
java.library.path), to use System.load which recieve full path.
I will test this locally on my machine next week.
[ Assuming it will work, I will try to do the change on seperate branch, do
we have somewhere in the code when we reference to the full path of the
artifact that was created?]
Regarding Windows failures-
1) Can you tell me- on that machine with Windows 7- Which VS are installed?
which Java version is used?
2) Can you send me the content of the target folder of the first failures?
I need only the build.log but the entire target folder to examine the
failure (FOr the MAC for example, I needed the surefile-reports folder)
Eyal
…On Thu, Apr 20, 2017 at 11:44 PM, Curtis Rueden ***@***.***> wrote:
I pushed all IT logs from that attempt to the same gist. Here is
it0001-executable.log
<https://gist.github.com/ctrueden/551535b1e1fdba5bcd499ff0e2035afe#file-it0001-executable-log>
.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#258 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AKT4ISdcQtScZTBs5DDSQkmkAqX9usBQks5rx8PFgaJpZM4LqcGA>
.
|
I'm sorry, I don't have that information anymore. The VM in question was probably infected with malware, so we shut it down permanently. And the person who set it up has left the project; I do not know its configuration specifically. If your Windows environment has all passing tests, then that is great. I wouldn't worry too much about it. The main thing is that we need to get Appveyor builds working properly, which means creating a maven-nar organization Appveyor account. |
|
Can you clarify what do you mean by "The main thing is that we need to get
Appveyor builds working properly, which means creating a maven-nar
organization Appveyor account" ?
Who can create it and what is needed for it?
How do we run the tests on MacOS and Linux?
Also- Can you approve the pull request? (I hope in a week or two create a
fix for the MacOS problem)
Basically it is replacement of Runtime.exec with process builder, than
since Java 7 update 21 is recommennded (Actually- it is recommended since
Java 1.5, but in 1.7 update 21 they made some changes), I add few issues
with Runtime.exec in the past and always move to process builder
helped.... (In this case- it gives me also the option of printing to the
logs the command executed, which makes my life much easier, without the
need to run -X)
http://www.oracle.com/technetwork/java/javase/7u21-relnotes-1932873.html#jruntime
"Alternatively, the preferred way to create operating systems processes
since JDK 5.0 is using java.lang.ProcessBuilder. The ProcessBuilder class
has a much more complete API for setting the environment, working directory
and redirecting streams for the process."
It might resolve- #225
Eyal
…On Fri, Apr 21, 2017 at 4:31 PM, Curtis Rueden ***@***.***> wrote:
Regarding Windows failures-
1. Can you tell me- on that machine with Windows 7- Which VS are
installed?
which Java version is used?
I'm sorry, I don't have that information anymore. The VM in question was
probably infected with malware, so we shut it down permanently. And the
person who set it up has left the project; I do not know its configuration
specifically.
If your Windows environment has all passing tests, then that is great. I
wouldn't worry too much about it. The main thing is that we need to get
Appveyor builds working properly, which means creating a maven-nar
organization Appveyor account.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#258 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AKT4IS2eOZeQ1PW7tjKccoAXAoHTF09jks5ryK_EgaJpZM4LqcGA>
.
|
|
Sorry- NOw I see that 225 is related to a different execution command.
A diffrent story (But even there- we can replace the makeExecutable
implementation with Java 1.7 implementation for setting the execute
permissions)
Eyal
…On Fri, Apr 21, 2017 at 5:28 PM, Eyal Goren ***@***.***> wrote:
Can you clarify what do you mean by "The main thing is that we need to get
Appveyor builds working properly, which means creating a maven-nar
organization Appveyor account" ?
Who can create it and what is needed for it?
How do we run the tests on MacOS and Linux?
Also- Can you approve the pull request? (I hope in a week or two create a
fix for the MacOS problem)
Basically it is replacement of Runtime.exec with process builder, than
since Java 7 update 21 is recommennded (Actually- it is recommended since
Java 1.5, but in 1.7 update 21 they made some changes), I add few issues
with Runtime.exec in the past and always move to process builder
helped.... (In this case- it gives me also the option of printing to the
logs the command executed, which makes my life much easier, without the
need to run -X)
http://www.oracle.com/technetwork/java/javase/7u21-
relnotes-1932873.html#jruntime
"Alternatively, the preferred way to create operating systems processes
since JDK 5.0 is using java.lang.ProcessBuilder. The ProcessBuilder class
has a much more complete API for setting the environment, working directory
and redirecting streams for the process."
It might resolve- https://github.com/maven-nar/
nar-maven-plugin/issues/225
Eyal
On Fri, Apr 21, 2017 at 4:31 PM, Curtis Rueden ***@***.***>
wrote:
> Regarding Windows failures-
>
> 1. Can you tell me- on that machine with Windows 7- Which VS are
> installed?
> which Java version is used?
>
> I'm sorry, I don't have that information anymore. The VM in question was
> probably infected with malware, so we shut it down permanently. And the
> person who set it up has left the project; I do not know its configuration
> specifically.
>
> If your Windows environment has all passing tests, then that is great. I
> wouldn't worry too much about it. The main thing is that we need to get
> Appveyor builds working properly, which means creating a maven-nar
> organization Appveyor account.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#258 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AKT4IS2eOZeQ1PW7tjKccoAXAoHTF09jks5ryK_EgaJpZM4LqcGA>
> .
>
|
See #275. |
|
And what is required to create such account?
Since we are open source project? (I assume you want organizational
account)?
Eyal
…On Thu, May 11, 2017 at 10:34 PM, Curtis Rueden ***@***.***> wrote:
Can you clarify what do you mean by "The main thing is that we need to get
Appveyor builds working properly, which means creating a maven-nar
organization Appveyor account" ?
See #275 <#275>.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#258 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AKT4IWznLjFomsgpBY_hNNOjkES-YQe9ks5r42KygaJpZM4LqcGA>
.
|
|
A couple of updates on the CI: 1) I finally created the And a question: for executing processes, would it make sense to lean on Apache Commons Exec? |
|
The Apache Commons Exec still rely on Runtime.getRuntime().exec, If only it
was using the ProcessBuilder instead...
Eyal
…On Sun, Nov 19, 2017 at 6:15 AM, Curtis Rueden ***@***.***> wrote:
A couple of updates on the CI: 1) I finally created the maven-nar
AppVeyor account; see the project page
<https://ci.appveyor.com/project/maven-nar/nar-maven-plugin>. The
configuration needs work, though. 2) I activated the Travis CI build as
well; see here <https://travis-ci.org/maven-nar/nar-maven-plugin>. So now
it should be doable to configure these systems to test PRs more thoroughly
cross-platform.
And a question: for executing processes, would it make sense to lean on Apache
Commons Exec <https://commons.apache.org/proper/commons-exec/>?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#258 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AKT4Ie6497ylbKOz9SCY8vOBXv1XqsgMks5s36tcgaJpZM4LqcGA>
.
|
|
Really sorry for my long silence on this PR, @eyalg1972. I am going to merge it, and we'll see how it goes with Travis CI. The AppVeyor CI still needs to be fixed, but that is definitely a separate issue from this change. |
|
Thanks!!
Eyal
…On Thu, Mar 1, 2018 at 6:24 PM, Curtis Rueden ***@***.***> wrote:
Merged #258 <#258>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#258 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AKT4IRBri6Svk2sdDW5dh0NDIbOfctZlks5taCCsgaJpZM4LqcGA>
.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instead of using the "Execute" class of ANT, I am now using the standard Java execution process using ProcessBuilder. Also- the executed command is printed so it is easy to see what might fail