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

Fix miss cache file problem on Jenkins concurrent install. #6535

Closed
wants to merge 7 commits into from
Closed

Fix miss cache file problem on Jenkins concurrent install. #6535

wants to merge 7 commits into from

Conversation

yanzhiwei147
Copy link
Contributor

@benasher44
Copy link
Member

@yanzhiwei147 at first glance this seems okay. Any thoughts on how we can test this?

@yanzhiwei147
Copy link
Contributor Author

Sorry, I try to add some test case for this issue.but unsuccessful.

@yanzhiwei147
Copy link
Contributor Author

It can be run normal on Jenkins concurrent install now for me.

lock_file_path = lock_path_for_pod(request, :name => name, :params => result.checkout_options)

File.new(lock_file_path, File::CREAT) unless File.exist? lock_file_path
File.open(lock_file_path) { |file| file.flock(File::LOCK_EX) && copy_and_clean(target, destination, spec) }
Copy link
Contributor

Choose a reason for hiding this comment

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

@yanzhiwei147 shouldn't we unlock the file once we are done?

Something akin to:

	    success = file.flock(mode)
	    if success
	      begin
	        yield file
	      ensure
	        file.flock(File::LOCK_UN)
	    end

Taken from https://www.safaribooksonline.com/library/view/ruby-cookbook/0596523696/ch06s13.html

@@ -107,6 +107,20 @@ def path_for_pod(request, slug_opts = {})
# the download options that should be used in constructing the
# cache slug for this request.
#
# @return [string] The lock file path for the Pod downloaded from the given
Copy link
Contributor

@dnkoutso dnkoutso Mar 17, 2017

Choose a reason for hiding this comment

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

nit: [String] (capital 'S')

@@ -156,7 +170,16 @@ def uncached_pod(request)

podspecs.each do |name, spec|
destination = path_for_pod(request, :name => name, :params => result.checkout_options)
copy_and_clean(target, destination, spec)

# add file lock
Copy link
Contributor

@dnkoutso dnkoutso Mar 17, 2017

Choose a reason for hiding this comment

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

"add file lock" does not add any context to any other programmer who reads this as to why a file lock is being used here. If the code is simple and self explanatory then I suggest removing this comment entirely, otherwise please expand it to explain why the lock is being used.

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

# @param [mode] mode
# the file lock mode.
#
# @return [boolean] The result of lock specific file and mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

[Boolean]

@@ -167,6 +190,26 @@ def uncached_pod(request)
end
end

# @param [file] file
Copy link
Contributor

Choose a reason for hiding this comment

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

[File]

@benasher44
Copy link
Member

I'm okay to merge. @dantoml @dnkoutso any final concerns? I'd prefer tests, but I'm not sure how to test file locks.

@dnkoutso
Copy link
Contributor

👍 from me.

@endocrimes
Copy link
Member

I would be really hesitant to merge this - we'd still be a long way from correctly supporting concurrent pod install's (because things like repo updating)

@yanzhiwei147
Copy link
Contributor Author

@dantoml Sorry, the pod repo update have the same problem,it resolved by using the repo(flock ~/.cocoapods-repo-master.lock pod repo update master).

@yanzhiwei147
Copy link
Contributor Author

@dnkoutso It can be merged?

@dnkoutso
Copy link
Contributor

dnkoutso commented Oct 13, 2017

@yanzhiwei147 rebase it please?

@dnkoutso
Copy link
Contributor

Closing due to inactivity.

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