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

Update scrapy & dependencies #22593

Merged
merged 4 commits into from Feb 16, 2017
Merged

Update scrapy & dependencies #22593

merged 4 commits into from Feb 16, 2017

Conversation

teh
Copy link
Contributor

@teh teh commented Feb 9, 2017

Motivation for this change
Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link

@teh, thanks for your PR! By analyzing the history of the files in this pull request, we identified @FRidh to be a potential reviewer.

@domenkozar
Copy link
Member

domenkozar commented Feb 9, 2017

cc @jozko

Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

haven't tested it but it looks good

@teh
Copy link
Contributor Author

teh commented Feb 15, 2017

@drewkett you ok with these changes?

@drewkett
Copy link

Looks fine to me, though I'd probably add a comment to the nix file explaining why the permissions patch is needed rather than having that information only be in a commit message.

I'm not using this package at the moment, so feel free to take over as maintainer if you'd like.

@teh
Copy link
Contributor Author

teh commented Feb 15, 2017

I left you in as the maintainer for now - I'm not directly using them either, had some unrelated code that needed a newer scrapy.

Updated according to your comment.

@teh
Copy link
Contributor Author

teh commented Feb 16, 2017

@FRidh - I think this is ready to merge.

@@ -0,0 +1,36 @@
{ self, buildPythonPackage, pkgs, lib }:
Copy link
Member

Choose a reason for hiding this comment

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

  • never pass in pkgs
  • python libraries are passed the individual python packages, not pythonPackages or self

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, PTAL

…code.

Scrapy is usually installed via pip where copying all permissions
makes sense. In Nix the files copied are owned by root and
readonly. As a consequence scrapy can't edit the project templates so

  scrapy startproject

fails.
@FRidh
Copy link
Member

FRidh commented Feb 16, 2017

thanks

@FRidh FRidh merged commit 624cd8a into NixOS:master Feb 16, 2017
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

5 participants