-
Notifications
You must be signed in to change notification settings - Fork 104
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
Add windows detection when executing arc
#270
Conversation
@colin-harms can you please sign the CLA? |
@colin-harms can you respond to the above? We'd love to get this included for our own purposes as we're experiencing the same issue with multi-platform builds. |
@withinfocus I signed it a while ago, I'm not sure why it's not recognising it. |
I closed and re-opened the PR for the checks to trigger again |
@colin-harms Maybe go to https://cla-assistant.io/uber/phabricator-jenkins-plugin?pullRequest=270 again and see if it sticks? |
I've tried chrome, chrome incognito, firefox ( which I never use ), signing out of CLA and back in. The signing page always switches from "Please sign ... " to "You have signed ... ". |
@kageiit can this be manually resolved then? |
Bump on this one. Would love for it to come through in the next plugin release. Can someone at Uber commandeer this and merge it themselves? |
@briankhsieh Any idea why the CLA is not being recognized? |
@withinfocus can you please resolve the conflicts? |
Hi can you try sign the CLA with username @ch4rms ? |
It finally worked!! |
@ch4rms can you please resolve the merge conflicts? |
Thanks @kageiit I was just fixing it up when I got the email notification :) |
This is in relation to #244
If the ApplyPatchTask is being instantiated as part of a build, it will check the JVM's "os.name" property to see if it's Windows. The "startsWith Windows" logic was lifted from common-lang's SystemUtils.IS_WINDOWS
I've been trying to get a proper unit test going for this, but because Computer.currentComputer() checks to see if the current thread is a Hudson Executor, it will fail in normal unit tests (as in ApplyPatchTaskTest)
If it runs as a build in the PhabricatorBuildWrapperTest class, it will actually check the values of "os.name", but I don't want to override it as part of the test, since it sets it globally across the jvm that is running the test.
I'm open to suggestions to create a lightweight test for this. I'm not very familiar with Jenkins plugin development, but I did run into that "arc" vs "arc.bat" problem, and this change solved it.