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

cocoapods: 1.5.3 -> 1.6.0 #55471

Merged
merged 1 commit into from Feb 22, 2019
Merged

cocoapods: 1.5.3 -> 1.6.0 #55471

merged 1 commit into from Feb 22, 2019

Conversation

lilyball
Copy link
Member

@lilyball lilyball commented Feb 9, 2019

Motivation for this change

Fixes #55458.
Fixes #55461.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

When testing ./result/bin/, there's a binary sandbox-pod that actually fails with the error

sandbox-exec: execvp() of '/nix/store/4r5mfkcgjabzc1k6qbdjbmyqa2kzvnr1-cocoapods-1.6.0/lib/ruby/gems/2.5.0/gems/cocoapods-1.6.0/bin/pod' failed: Operation not permitted

But this same error occurs with the current v1.5.3. I'm tempted to say we should stop vending sandbox-pod as a binary since it doesn't seem to work, but that's not actually related to this version update.

/cc @peterromfeldhk

@peterromfeldhk
Copy link
Contributor

i still like to clean up ~/.gem on my osx :)
ill look into it tomorrow

@lilyball
Copy link
Member Author

The modified update script doesn't touch ~/.gem so it shouldn't be deleting it.

@lilyball
Copy link
Member Author

lilyball commented Feb 10, 2019

That said, I didn't remove the optional clean action.

@lilyball
Copy link
Member Author

Oops yes I did. I forgot. Well, the point remains that the script doesn't have anything to do with ~/.gem.

@peterromfeldhk
Copy link
Contributor

peterromfeldhk commented Feb 11, 2019

how about? worked fine without asking for password and also you should be sure that everything is fresh installed (and removed)

#!/usr/bin/env nix-shell
#! nix-shell -i bash -p bash ruby bundler bundix

BUNDIX_CACHE=$(mktemp -d)

rm Gemfile.lock
bundler install --path $BUNDIX_CACHE
bundix --bundle-pack-path=$BUNDIX_CACHE

rm -rf $BUNDIX_CACHE
rm -rf .bundle

@lilyball
Copy link
Member Author

AFAICT there's no need to actually run bundler install, all we really need is the Gemfile.lock which is what bundler lock gives us.

The bundix README does suggest it looks in the bundler cache for the gems and falls back to fetching from remotes if it can't find the gem. I'm not sure if there's a benefit to this approach, but if so, the README suggests this should be generated via bundler package, not bundler install.

And finally, if we are going this route, we may as well just use a relative directory instead of making a temporary one. If you do use a temporary directory, at the very least you need quotes around $BUNDIX_CACHE in case the configured $TMPDIR has a space in it.

@peterromfeldhk
Copy link
Contributor

ok i never looked that much into the workings of bundix i just assumed it needs the installed gems since it has it in -h
im fine with it as long as it works and does not leave some lingering state around

@peterromfeldhk
Copy link
Contributor

peterromfeldhk commented Feb 11, 2019

i tried it with your suggestions and it still works fine and i can be sure that there is no lingering state in my system left over. and i also like how explicit it is :)

#!/usr/bin/env nix-shell
#! nix-shell -i bash -p bash ruby bundler bundix

BUNDIX_CACHE="$(mktemp -d)"

rm Gemfile.lock
bundler package --path "$BUNDIX_CACHE"
bundix --bundle-pack-path="$BUNDIX_CACHE"

rm -rf "$BUNDIX_CACHE"
rm -rf .bundle

@peterromfeldhk
Copy link
Contributor

also first glance google for TEMPDIR and spaces it seems that it should not contain spaces... but better safe then sorry

@lilyball
Copy link
Member Author

Ok I updated the update script to match your version.

@lilyball
Copy link
Member Author

@peterromfeldhk Anything holding up this PR?

@peterromfeldhk
Copy link
Contributor

@lilyball everything looks good to me.

@lilyball
Copy link
Member Author

It's not clear to me how to get this merged. Looks like it doesn't even have the right labels yet (the manual says it should have the 8.has: package (update) label).

@Mic92
Copy link
Member

Mic92 commented Feb 22, 2019

@GrahamcOfBorg build cocoapods

@Mic92 Mic92 merged commit 1c349cb into NixOS:master Feb 22, 2019
@Mic92
Copy link
Member

Mic92 commented Feb 22, 2019

Thanks!

@lilyball lilyball deleted the cocoapods_1.6.0 branch February 22, 2019 19:42
@lilyball
Copy link
Member Author

Ironically, cocoapods 1.6.1 was just published 21 hours ago, so time to update again.

@lilyball lilyball mentioned this pull request Feb 22, 2019
10 tasks
@lilyball
Copy link
Member Author

Submitted the 1.6.1 update as #56221

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

Successfully merging this pull request may close these issues.

None yet

4 participants