Skip to content

Comments

simplify flake#6538

Merged
roberth merged 1 commit intoNixOS:masterfrom
zimbatm:simplify-flake
Feb 20, 2023
Merged

simplify flake#6538
roberth merged 1 commit intoNixOS:masterfrom
zimbatm:simplify-flake

Conversation

@zimbatm
Copy link
Member

@zimbatm zimbatm commented May 16, 2022 •
edited
Loading

Basically finishing #6196

/cc @Ericson2314

@zimbatm zimbatm force-pushed the simplify-flake branch 4 times, most recently from c6cc7f4 to 5d53ef3 Compare May 17, 2022 14:16
Ericson2314
Ericson2314 approved these changes May 23, 2022
View reviewed changes
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

Thank you!!

@Ericson2314 Ericson2314 mentioned this pull request May 23, 2022
Closed
@zimbatm zimbatm requested a review from edolstra May 24, 2022 11:56
@zimbatm
Copy link
Member Author

zimbatm commented May 24, 2022

@edolstra any objections to merge?

@Ericson2314
Copy link
Member

Ericson2314 commented Jun 8, 2022

Yes please merge this.

@nixos-discourse
Copy link

nixos-discourse commented Dec 19, 2022

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

https://discourse.nixos.org/t/2022-12-16-nix-team-meeting-minutes-17/24120/1

Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Jan 23, 2023
@Ericson2314
Try to fix NixOS#7669
3542207 
The issue *seems* to be the cross jobs, which are missing the `CXXFLAGS`
needed to get rapidcheck.

PR NixOS#6538 would be really nice to resurrect which will prevent the
`configureFlags` from going out of sync between the regular build and
the cross build again.
@Ericson2314 Ericson2314 mentioned this pull request Jan 23, 2023
Merged
7 tasks
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Jan 23, 2023
@Ericson2314
Try to fix NixOS#7669
836daad 
The issue *seems* to be the cross jobs, which are missing the `CXXFLAGS`
needed to get rapidcheck.

PR NixOS#6538 would be really nice to resurrect which will prevent the
`configureFlags` from going out of sync between the regular build and
the cross build again.
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Jan 23, 2023
@Ericson2314
Try to fix NixOS#7669
a91709a 
The issue *seems* to be the cross jobs, which are missing the `CXXFLAGS`
needed to get rapidcheck.

PR NixOS#6538 would be really nice to resurrect which will prevent the
`configureFlags` from going out of sync between the regular build and
the cross build again.
github-actions bot pushed a commit that referenced this pull request Jan 24, 2023
@Ericson2314 @github-actions
Try to fix #7669
e60ee9e 
The issue *seems* to be the cross jobs, which are missing the `CXXFLAGS`
needed to get rapidcheck.

PR #6538 would be really nice to resurrect which will prevent the
`configureFlags` from going out of sync between the regular build and
the cross build again.

(cherry picked from commit a91709a)
@roberth
Copy link
Member

roberth commented Jan 24, 2023

I think this PR should be assigned to a single person who takes full ownership.
If I recall correctly we decided to review together - hence no assignee, but I think this is unnecessary. It is just a refactoring, and anyone can rebase it and then ascertain that either the final attributes remain unchanged, or are perhaps slightly improved somehow.
As this change is sensitive to conflicts, it's better to act decisively rather than go back and forth to let this stall again.
I'd be happy to complete this task.

@zimbatm
Copy link
Member Author

zimbatm commented Jan 24, 2023

The merge conflicts are why I haven't been rebasing it anymore. I was waiting to get a clear signal that this would get merged fairly quickly after. So please tell me "as the Nix team ..." and then I will re-do it.

@Ericson2314
Copy link
Member

Ericson2314 commented Jan 25, 2023

I think we moved it to "in review" in triage without posting a thing here in hopes someone would fix it. I will move back to in discussion so we can get @zimbatm the "clear signal".

@Ericson2314
Copy link
Member

Ericson2314 commented Jan 25, 2023

Or alternatively someone on the @nixos/nix-team maybe feels comfortable approving the concept here async.

@Ericson2314 Ericson2314 mentioned this pull request Jan 25, 2023
Merged
7 tasks
@roberth roberth mentioned this pull request Jan 31, 2023
Open
@roberth
Copy link
Member

roberth commented Feb 3, 2023

@zimbatm The team has approved this change. I will review and merge it as soon as possible.

@Ericson2314 Ericson2314 closed this Feb 4, 2023
@Ericson2314 Ericson2314 reopened this Feb 4, 2023
@Ericson2314
Copy link
Member

Ericson2314 commented Feb 4, 2023

(sorry hit wrong button!)

@roberth roberth mentioned this pull request Feb 7, 2023
Closed
14 tasks
@roberth
Copy link
Member

roberth commented Feb 10, 2023

@zimbatm
Friendly reminder that this has been approved by the Nix team. Would you like to update this PR?

@zimbatm zimbatm force-pushed the simplify-flake branch 4 times, most recently from fa164ac to 943a0f5 Compare February 19, 2023 17:28
@zimbatm
Copy link
Member Author

zimbatm commented Feb 19, 2023 •
edited
Loading

ok, this is ready for a round of review. I will squash the two commits afterwards.

Ericson2314
Ericson2314 reviewed Feb 19, 2023
View reviewed changes
Ericson2314
Ericson2314 reviewed Feb 19, 2023
View reviewed changes
Ericson2314
Ericson2314 reviewed Feb 19, 2023
View reviewed changes
Ericson2314
Ericson2314 approved these changes Feb 19, 2023
View reviewed changes
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

It looks good now! Further cleanups / de-duping can be done in follow-up PRs.

roberth
roberth approved these changes Feb 19, 2023
View reviewed changes
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Checked by evaluating some of the attributes. No changes in the default package or alternate stdenv packages. Small improvements to the cross package, such as separate debug info.

d() { nix-diff "$(nix eval ".?ref=upstream/master#$1" | jq -r .)" "$(nix eval ".#$1" | jq -r .)"; }

@roberth
Copy link
Member

roberth commented Feb 19, 2023

Note to self: have a look at the hydra comparison after this is merged.

@zimbatm
Copy link
Member Author

zimbatm commented Feb 19, 2023

couldn't resist to add two more noop changes :)

@roberth
Copy link
Member

roberth commented Feb 19, 2023

Lovely! Could you squash the fixups?

@zimbatm
Copy link
Member Author

zimbatm commented Feb 20, 2023 •
edited
Loading

done (there was one merge conflict in the manual)

@zimbatm
Copy link
Member Author

zimbatm commented Feb 20, 2023

caused by f2e4279

@Ericson2314 @zimbatm
Nix's own flake: Dedup and memoize more
d7a4f08 
- `nixpkgsFor` does all of native, static, cross, and the different stdenvs.

- The main Nix derivation is no longer duplicated for static.

- DRY nixpkgs.lib and lib.genAttrs calls.
@zimbatm
Copy link
Member Author

zimbatm commented Feb 20, 2023

fixed

@roberth roberth merged commit de71483 into NixOS:master Feb 20, 2023
@zimbatm zimbatm deleted the simplify-flake branch February 20, 2023 12:47
@Ericson2314 Ericson2314 mentioned this pull request Feb 21, 2023
Merged
7 tasks
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Feb 21, 2023
@Ericson2314
Fix isStatic arguments to commonDeps
16111aa 
Some dependencies supposed to be skipped in the cross build, along with
not using the gold linker. But in NixOS#6538
this was accidentally not preserved.

Also since NixOS#6538 we saw some new
aarch64-linux static build failures. This is a first attempt to try to
fix those failures. If this is not sufficient, there are other things we
can try next.
@Ericson2314 Ericson2314 mentioned this pull request Feb 21, 2023
Merged
7 tasks
@cole-h cole-h mentioned this pull request Mar 2, 2023
Merged
7 tasks
@nixos-discourse
Copy link

nixos-discourse commented Apr 21, 2023

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

https://discourse.nixos.org/t/nix-team-report-2022-10-2023-03/27486/1

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

Reviewers

@Ericson2314 Ericson2314 Ericson2314 approved these changes

@edolstra edolstra Awaiting requested review from edolstra edolstra is a code owner

@thufschmitt thufschmitt Awaiting requested review from thufschmitt

@fricklerhandwerk fricklerhandwerk Awaiting requested review from fricklerhandwerk

+1 more reviewer

@roberth roberth roberth approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@roberth roberth

Labels

None yet

Projects

Nix team
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zimbatm @Ericson2314 @nixos-discourse @roberth