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

Windows support - ensure pod install/update will succeed. #8189

Merged
merged 1 commit into from Nov 6, 2018

Conversation

Projects
None yet
6 participants
@davidair
Copy link
Contributor

davidair commented Oct 15, 2018

This should address issues such as #8055 and #8077.

For this to work, a developer would need to install git and curl and make sure they are available in the PATH.

@davidair davidair force-pushed the davidair:master branch from 367eee2 to 88c80f7 Oct 15, 2018

@davidair

This comment has been minimized.

Copy link
Contributor Author

davidair commented Oct 15, 2018

Note: this also requires CocoaPods/Core#466 for pod update to work

@@ -8,6 +8,10 @@ To install release candidates run `[sudo] gem install cocoapods --pre`

##### Enhancements

* Ensuring pod install/update can run on Windows.
[David Airapetyan](https://github.com/davidair)

This comment has been minimized.

@paulb777

paulb777 Oct 16, 2018

Member

indentation

@davidair

This comment has been minimized.

Copy link
Contributor Author

davidair commented Oct 16, 2018

When running "bundle exec rake spec:all", not seeing the failures that were reported by Travis. Are there any flaky tests?

@paulb777

This comment has been minimized.

Copy link
Member

paulb777 commented Oct 16, 2018

Looks like master broke earlier today - https://travis-ci.org/CocoaPods/CocoaPods/builds and the fixing PR is not yet merged - #8195

@@ -49,6 +49,10 @@ def self.execute_command(executable, command, raise_on_failure = true)
bin = which!(executable)

command = command.map(&:to_s)
if bin.end_with?('tar.exe')

This comment has been minimized.

@segiddins

segiddins Oct 17, 2018

Member

this needs a test

This comment has been minimized.

@segiddins

segiddins Oct 17, 2018

Member

also, shouldn't this be File.basename(bin) == "tar.exe" ?

@@ -91,6 +95,9 @@ def self.which(program)
paths.uniq!
paths.each do |path|
bin = File.expand_path(program, path)
if Gem.win_platform?

This comment has been minimized.

@segiddins
@davidair

This comment has been minimized.

Copy link
Contributor Author

davidair commented Oct 17, 2018

@segiddins - writing proper tests will be complicated.

I never got a development environment set on up on Windows (I ended up running tests on macOS and patching my changes to the live copy of CocoaPods on the Windows VM).

Getting tests working will probably be a significant effort:

  • I'll need to figure out and set up Travis on Windows (first on my fork)
  • I'll need to find a way of doing an unattended install of the pre-requisites (git, curl)
  • I'll have to set up the Ruby Gems environment to be able to run tests

I was hoping to keep the Windows support "experimental" without getting the proper test coverage.

Please let me know where you'd like to take this - I am not opposed to creating the Windows test suite, but I am afraid it can take us down a pretty deep rabbit hole.

@segiddins

This comment has been minimized.

Copy link
Member

segiddins commented Oct 17, 2018

You could also stub Gem.win_platform? and test that way

@davidair davidair force-pushed the davidair:master branch from a775543 to d8e884c Oct 20, 2018

@davidair

This comment has been minimized.

Copy link
Contributor Author

davidair commented Oct 20, 2018

Rebased against upstream/master again to get rid of a merge/multiple commits

@davidair davidair force-pushed the davidair:master branch from d8e884c to 337ef32 Oct 20, 2018

@@ -18,6 +18,9 @@ To install release candidates run `[sudo] gem install cocoapods --pre`
## 1.6.0.beta.2 (2018-10-17)

##### Enhancements
* Ensuring pod install/update can run on Windows.

This comment has been minimized.

@dnkoutso

dnkoutso Oct 22, 2018

Contributor

nit press enter at the beginning of the line

This comment has been minimized.

@davidair

davidair Oct 23, 2018

Author Contributor

Added the new line

@davidair davidair force-pushed the davidair:master branch from 6635796 to 7780c29 Oct 24, 2018

@davidair

This comment has been minimized.

Copy link
Contributor Author

davidair commented Oct 24, 2018

Squashed the commits to make this ready to merge.

davidair added a commit to davidair/Core that referenced this pull request Oct 25, 2018

Windows support
Forks are not supported on windows - making sure we don't crash

CocoaPods/CocoaPods#8189 has the rest of support
@hispeedimagins

This comment has been minimized.

Copy link

hispeedimagins commented Oct 29, 2018

Thank you so much for this pull. I edited the source myself to get my end working.
though you have change one more line of code. Sorry I do not remember the file or the line number, but
a command something like this env -u GIT_CONFIG git is executed and you will get an error about origin url. I removed the part before git and it works now.
@davidair

@paulb777

This comment has been minimized.

Copy link
Member

paulb777 commented Nov 4, 2018

@dnkoutso Will you add this PR to a milestone?

@dnkoutso dnkoutso added this to the 1.7.0 milestone Nov 6, 2018

@dnkoutso dnkoutso merged commit f9904ab into CocoaPods:master Nov 6, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@davidair

This comment has been minimized.

Copy link
Contributor Author

davidair commented Nov 6, 2018

@dnkoutso - Thank you! Do you think CocoaPods/Core#466 could also be added to 1.7.0? It is a complementary PR that is also needed.

davidair added a commit to davidair/Core that referenced this pull request Nov 6, 2018

Windows support
Forks are not supported on windows - making sure we don't crash

CocoaPods/CocoaPods#8189 has the rest of support
@davidair

This comment has been minimized.

Copy link
Contributor Author

davidair commented Nov 6, 2018

@hispeedimagins - I couldn't find GIT_CONFIG in the codebase - I will be doing more testing, but let me know if you find where you had to make the change (or feel free to submit a PR of your own!)

@foxatplay

This comment has been minimized.

Copy link

foxatplay commented Feb 5, 2019

@hispeedimagins You might have saved my life... pointed me in the right direction.
@davidair the file is in cocoapods-core/source.rb
After that I get another no such file or path error about /dev/null... obviously because it doesn't exist. you can then replace that simply with nul (which is the equivalent in windows).
This is in cocoapods-stats/sender.rb

@hispeedimagins

This comment has been minimized.

Copy link

hispeedimagins commented Feb 5, 2019

@davidair sorry for the late reply, I did not receive any notification for your reply.

I'll try posting a or, hopefully I can do it with clean code,

till then for others who stumble here,

@foxatplay is right, the file is sources.rb

This is the code to change at line 448 if no more changes have happened.

def repo_git(args, include_error: false)
#command = "env -u GIT_CONFIG git -C "#{repo}" " << args.join(' ')
command = "git -C "#{repo}" " << args.join(' ')
command << ' 2>&1' if include_error
(#{command} || '').strip
end

@davidair

This comment has been minimized.

Copy link
Contributor Author

davidair commented Feb 12, 2019

Thank you @hispeedimagins and @foxatplay, I'll take a look when 1.7 Beta is out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment