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.

Sign up for GitHub

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

flutter: move build-support/flutter to flutter directory #332623

Merged
merged 1 commit into from
Sep 13, 2024
Merged

flutter: move build-support/flutter to flutter directory #332623

merged 1 commit into from
Sep 13, 2024
+1 −1

Conversation

AndersonTorres
Copy link
Member

Description of changes

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@AndersonTorres AndersonTorres marked this pull request as ready for review August 6, 2024 01:39
@eclairevoyant
Copy link
Contributor

What's the reasoning behind?

@RossComputerGuy
Copy link
Member

Yeah and if we do this, will rust and other languages/frameworks be moved out of build-support?

@AndersonTorres
Copy link
Member Author

AndersonTorres commented Aug 6, 2024 •
edited
Loading

What's the reasoning behind?

Mucho texto:

RFC 140 promotes self-contained package directories and strongly discourages files scattered along the filesystem.
Indeed it explicitly prohibits a by-name package to refer to file paths outside its own directory.

Things like ../../../../pkgs/. . . are not allowed in this new framework.
(By the way, weren't you never trapped by counting how many superdirectories you should navigate in order to reach such files?)

Further, with the advent of both RFCs 140 and 146, the classification-based package hierarchy is deprecated and should be phased out.

Further, and more to the point here, no other files in the whole Nixpkgs call ../../../build-support/flutter except pkgs/development/compilers/flutter/default.nix.
This is just a one-line diff, way easier to understand when compared to the same thing that happened with vim and emacs ecosystems.

Yeah and if we do this, will rust and other languages/frameworks be moved out of build-support?

Well, why not?
Supposing this is doable, why not?

It is way cleaner and easier to reason about an ecosystem/framework knowing that all its relevant files are concentrated in a single directory tree rather than scattered among a wild, obsolete directory hierarchy.

Further, there are some precedents for this approach (yeah, all mine, it is a bit biased I know):

@philiptaron philiptaron removed their request for review September 4, 2024 18:36
@AndersonTorres AndersonTorres force-pushed the flutter-concentrate branch 2 times, most recently from b7d49f5 to c289f68 Compare September 6, 2024 12:38
@AndersonTorres
flutter: move build-support/flutter to flutter directory
a0f3c9a 
Rationale:

With the advent of RFC 140, Nixpkgs becomes to promote self-contained package
directories and strongly discourages scattering along the old and deprecated
filesystem hierarchy.

Further, `build-support/flutter/*` is not referenced by any other expressions in
Nixpkgs besides flutter itself.

Further, concentrating all Flutter-related toolset below a same directory
hierarchy (arguably?) eases maintenance and future migrations.
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1950

@thiagokokada thiagokokada merged commit 303f08f into NixOS:master Sep 13, 2024
25 checks passed
@AndersonTorres AndersonTorres deleted the flutter-concentrate branch September 13, 2024 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@superherointj superherointj superherointj approved these changes

@Aleksanaa Aleksanaa Aleksanaa approved these changes

@RossComputerGuy RossComputerGuy Awaiting requested review from RossComputerGuy

Assignees
No one assigned
Labels
6.topic: flutter 10.rebuild-darwin: 0 10.rebuild-linux: 0 12.approvals: 2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@AndersonTorres @eclairevoyant @RossComputerGuy @nixos-discourse @superherointj @Aleksanaa @thiagokokada @wegank