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

Fixing for windows #77

Merged
merged 6 commits into from
Feb 14, 2017
Merged

Fixing for windows #77

merged 6 commits into from
Feb 14, 2017

Conversation

scphantm
Copy link
Contributor

@scphantm scphantm commented Feb 8, 2017

Expands on Vampire's code and fixes more bugs associated with running on windows

Vampire and others added 5 commits April 7, 2016 14:28
…FixingForWindows

# Conflicts:
#	src/test/groovy/nebula/test/ChangingTestDirSpec.groovy
#	src/test/groovy/nebula/test/functional/GradleRunnerSpec.groovy

Also made the tests run on windows and should run on linux.  Need to test that still.
@scphantm
Copy link
Contributor Author

scphantm commented Feb 8, 2017

As i said before, you may want to consider adding appveyor to the checks routine for all of Nebula. Unfortunately, I'm one of the poor lost souls that have to run on Winblows and any time i crack Nebula open, I encounter a Winblows related error. I think I got them all out of this, but the gradle tests stuff is horribly old, don't want to get into that and very few unit tests check for gradle 3.x compatibility. That stuff is beyond me at the moment tho. Its a shame because I have some major overhauls that I could do to this library to significantly enhance its capabilities, Its just got to be brought up to the 3.x stuff, my stuff would be hard pressed to work on 2.x

@scphantm
Copy link
Contributor Author

scphantm commented Feb 8, 2017

And if we could please get this in as quickly as possible, i would appreciate it. I had to make these changes to address bugs I found in Nebula-Publish. I can't get those changes merged until these changes are merged and released and the Nebula-Publish issues have me dead in the water at the moment. Please let me know what i can do to speed this up. Thanks

@scphantm
Copy link
Contributor Author

scphantm commented Feb 8, 2017

It looks like when you merge this, you will also have to update nebula-plugin-plugin to pull the correct version. In my testing of nebula-publish i have to do some pretty nasty things to get around plugin-plugins dependencies.

@scphantm
Copy link
Contributor Author

Its been 6 days, can someone give me a status on this MR? Code review? Something? This has me at a stand still and I'm running out of busy work. Thanks

@scphantm
Copy link
Contributor Author

in case your wondering, i need this merged and nebula-plugin updated so i can finish this PR nebula-plugins/nebula-publishing-plugin#71

@szpak
Copy link
Contributor

szpak commented Feb 14, 2017

@scphantm In this project PRs quite often wait long time before being (eventually) merged. Waiting 6 months for my #68 to be merged I've switched my project to a custom JitPack version. Just as a workaround better than snapshots... Maybe it would be suitable also for you.

@scphantm
Copy link
Contributor Author

thanks @szpak , im hoping i can avoid that, as everything is so interconnected, i would basically have to pull nearly all of nebula in house and start running my own builds. I REALLY don't want to do that.

@rspieldenner
Copy link
Contributor

Our new plugins are planned to use gradle testKit for testing so as we migrate more and more of them over to that, this project will become more and more helpers for testKit.

@scphantm
Copy link
Contributor Author

That's great. But can we get this issue clears in the mean time so I can finish fixing publish?

.gitignore Outdated
@@ -8,3 +8,4 @@ gradle.properties
*.iml
*.iws
out/
test/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this, I'll fix the test that is writing outside of the build directory

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

k

@@ -23,7 +24,12 @@ class SpecifiedGradleVersionIntegrationSpec extends IntegrationSpec {
when:
def result = runTasksSuccessfully('build')
then:
result.standardOutput.contains("gradle/$requestedGradleVersion/taskArtifacts")
if (Os.isFamily(Os.FAMILY_WINDOWS)){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a spock construct to do this. I'd rather have 2 separate tests that require different OS's then an if block in the test.

http://spockframework.org/spock/docs/1.0/extensions.html

IgnoreIf block

os Information about the operating system (see spock.util.environment.OperatingSystem)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh i know the ignoreif annotation, i just hate the dup code is all. but if you want two tests, no problem, i can do that. And as i write this, i just realized how to pull it off. give me a few minutes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so my brilliant idea wasn't so brilliant. its 2 tests now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants