Skip to content

expo-cli: init + implement overrideNodeAttrs - #96509

Merged
jtojnar merged 2 commits into
NixOS:masterfrom
jtojnar:expo-cli
Oct 7, 2020
Merged

jtojnar merged 2 commits into
NixOS:masterfrom
jtojnar:expo-cli

Conversation

@jtojnar

@jtojnar jtojnar commented Aug 27, 2020 •

Copy link
Copy Markdown
Member
Motivation for this change

Closes: #93399

Had to implement overrideNodeAttrs to filter out optional @expo/traveling-fastlane-darwin dependency that was intentionally aborting the build on Linux.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
    • Successfully ran react-native app on Android using this
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@jtojnar

jtojnar commented Aug 27, 2020

Copy link
Copy Markdown
Member Author

cc @FRidh who implemented overridePythonAttrs I based this on

cc @Scriptkiddi who was also interested in this

@ofborg ofborg Bot added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Aug 28, 2020
@FRidh

FRidh commented Aug 28, 2020

Copy link
Copy Markdown
Member

While I am not sure whether it is the correct direction to add these language/framework-specific overrides (see NixOS/rfcs#67 (comment)) I do think the implementation in #87394 would be preferred over us implementing our own ones every time.

@jtojnar

jtojnar commented Aug 28, 2020

Copy link
Copy Markdown
Member Author

Thanks, I was trying to find the PR but did not realize it was closed. I agree with you but in the meantime I would like a working, if only temporary solution.

What do you think about gating it on explicit acknowledgement that the function can be dropped at any time?

@FRidh

FRidh commented Aug 29, 2020 •

Copy link
Copy Markdown
Member

Well, I say we should just use that PR. Having those override functions does not preclude having a common override function which I proposed (that could be say overrideArgs).

@ryantm ryantm added 2.status: merge conflict This PR has merge conflicts with the target branch and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Oct 3, 2020
@ofborg ofborg Bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Oct 7, 2020
@ofborg
ofborg Bot requested a review from midchildan October 7, 2020 19:28
@jtojnar

jtojnar commented Oct 7, 2020

Copy link
Copy Markdown
Member Author

I think I will use the local internal function for now, rather than switching to a public API that would be deprecated after RFC 67. I will port to that once it gets accepted.

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

Labels

10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Packaging request: expo

3 participants