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

Jump to bottom

cocoapods-beta: init at 1.7.0.beta.3 #59252

Merged
merged 2 commits into from Apr 17, 2019
Merged

cocoapods-beta: init at 1.7.0.beta.3 #59252

merged 2 commits into from Apr 17, 2019

Conversation

lilyball
Copy link
Member

@lilyball lilyball commented Apr 10, 2019 •
edited

Motivation for this change

cocoapods-beta is an alternative attribute for the cocoapods package that provides the latest beta instead of the stable version.

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 nix-review --run "nix-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.

matthiasbeyer
matthiasbeyer requested changes Apr 10, 2019
View reviewed changes
Copy link
Contributor

@matthiasbeyer matthiasbeyer left a comment

Choose a reason for hiding this comment

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

The commit message does not follow the guidelines. Sorry.

@lilyball
Copy link
Member Author

@matthiasbeyer Are you talking about the subject, or the body? I'm not aware of guidelines around the commit message body, and the subject should follow guidelines.

I know our guidelines say to use the package name, not the attribute name, but I went back and checked the commit that introduced nodejs_11_x (because that's the same scenario, where the package already exists but we're introducing a different attribute for a new version) and it did what I did here, which is use the attribute name with the init style.

@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/rubygem-conflicts-with-ruby-packages/2636/5

@lilyball
Make gemdir optional for bundlerApp
8e9796c 
Like `bundlerEnv`, the `gemdir` parameter to `bundlerApp` can be omitted
if all 3 of `gemfile`, `lockfile`, and `gemset` are provided.
@lilyball
Copy link
Member Author

I've just updated this PR 2 with 2 changes:

  • Switched to bundlerApp so we stop linking our dependencies into the profile, and can get rid of the broken sandbox-pod executable.
  • Tweaked bundlerApp to make the gemdir parameter optional just like it is in bundlerEnv, as it's unnecessary if we're providing all 3 of gemfile, lockfile, and gemset.

@lilyball lilyball mentioned this pull request Apr 10, 2019
Merged
10 tasks
@matthiasbeyer
Copy link
Contributor

The commit message is init: cocoapods-beta at 1.7.0.beta.3 but should be cocoapods-beta: init at 1.7.0.beta.3.

alyssais
alyssais reviewed Apr 11, 2019
View reviewed changes
Copy link
Member

@alyssais alyssais left a comment

Choose a reason for hiding this comment

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

bundlerApp change looks good :)

pkgs/development/mobile/cocoapods/default.nix Outdated Show resolved Hide resolved
@lilyball
Copy link
Member Author

@matthiasbeyer Oh oops, you're right. I'll fix it.

@lilyball
Copy link
Member Author

Both issues (commit message and comma) have been fixed.

@lilyball lilyball changed the title init: cocoapods-beta at 1.7.0.beta.3 cocoapods-beta: init at 1.7.0.beta.3 Apr 12, 2019
alyssais
alyssais reviewed Apr 12, 2019
View reviewed changes
pkgs/development/mobile/cocoapods/update Outdated
rm -rf "$BUNDIX_CACHE"
rm -rf .bundle
bundler lock --update
# For some reson `bundler lock` doesn't take a `--gemfile` flag
Copy link
Member

Choose a reason for hiding this comment

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

Does the BUNDLE_GEMFILE environment variable work?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just tested, looks like the answer is "no". I have to delete Gemfile-beta.lock first as apparently bundler lock --update won't backdate a lockfile despite the documentation claiming that flag makes it ignore the current lockfile (🤦🏻‍♀️) but after doing that, passing env BUNDLE_GEMFILE=Gemfile-beta bundler lock --update --lockfile=Gemfile-beta.lock produces a lockfile that's clearly coming from Gemfile.

Copy link
Member

Choose a reason for hiding this comment

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

Grr, okay. If you open a Bundler bug and add it to the comment, I'm happy to merge this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh. I just tested again to make sure and it actually seems like it's working now. I don't know what I did wrong before, since I double-checked my work last time, but if it really does work, I'll update this script to use that.

Copy link
Member

Choose a reason for hiding this comment

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

Woo!

@lilyball
cocoapods-beta: init at 1.7.0.beta.3
ae22f7d 
cocoapods-beta is an alternative attribute for the cocoapods package
that provides the latest beta instead of the stable version.

Also switch to `bundlerApp` so we stop polluting the profile with our
gem dependencies and can get rid of the broken `sandbox-pod` executable.
@lilyball
Copy link
Member Author

@alyssais PR updated to use BUNDLE_GEMFILE instead of the shell trickery.

@alyssais alyssais merged commit 69ec9bd into NixOS:master Apr 17, 2019
@alyssais
Copy link
Member

Thanks @lilyball

@lilyball lilyball deleted the cocoapods-beta branch April 17, 2019 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alyssais alyssais alyssais left review comments

@matthiasbeyer matthiasbeyer matthiasbeyer requested changes

@zimbatm zimbatm Awaiting requested review from zimbatm

Assignees
No one assigned
Labels
6.topic: ruby 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 0 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@lilyball @nixos-discourse @matthiasbeyer @alyssais @GrahamcOfBorg