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

Add inline prediction option mirroring the capitalization option #966

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

347Online
Copy link

No description provided.

@emilazy
Copy link
Collaborator

emilazy commented Jun 13, 2024

Thank you for the contribution! Looks like the tests are failing, unfortunately.

@347Online
Copy link
Author

This is true. I haven't figured out why yet. I've been meaning to take another look but then I got busy 🙃

Copy link
Collaborator

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

Looks like all the tests are passing now!

Sorry to nitpick, but we like to maintain a linear history (no merges from master into pull requests) and keep fix‐ups in the same commit (to make git bisect work better and scanning the commit log cleaner). Could you rebase on the upstream commit and squash your two commits together so I can merge this?

$ git rebase b2ee0b3c03b06bd70e4280c65ab8803c3456bb0f
$ git rebase -i HEAD~2
(change "pick" to "f"(ixup) on the fix commit)

@347Online
Copy link
Author

Ah, alright, no problem. I'll do that when I get back to my computer.

emilazy and others added 11 commits June 15, 2024 22:26
With 23.05 support dropped, this was pinned to an unsupported version.
These deprecated versions were already made unsupported by LnL7#932.
All supported Nixpkgs versions now support this.

This reverts commit a5b0958.
Process substitution behaves better with variables and it's good
practice to use `lib.escapeShellArgs`.
As far as I can tell, this isn't required to get fonts to work on
NixOS, so we shouldn't require it on nix-darwin either, even if the
implementations are superficially similar.
Stricter launchd -> StartCalendarInterval type:

- Verify that the integers passed to `Minute`, `Hour`, etc. are within
  range.

- When provided, the value for StartCalendarInterval must be a non-empty
  list of calendar intervals and must not contain duplicates entries
  (throw an error otherwise).

- For increased flexibility and backwards-compatibility, allow an
  attrset to be passed as well (which will be type-checked and is
  functionally equivalent to passing a singleton list). Allowing an
  attrset or list is precisely in-line with what `launchd.plist(5)`
  accepts for StartCalendarInterval.

Migrate `nix.gc.interval` and `nix.optimise.interval` over to use this
new type, and update their defaults to run weekly instead of daily.

Create `modules/launchd/types.nix` file for easier/modular use of
launchd types needed in multiple files.

Documentation:

- Update and improve wording/documentation of launchd's
  `StartCalendarInterval`.

- Improve wording/documentation of `nix.gc.interval` and
  `nix.optimise.interval` ("time interval" can be misleading as it's
  actually a "calendar interval"; e.g. `{ Hour = 3; Minute = 15;}`
  runs daily, not every 3.25 hours).
@347Online
Copy link
Author

347Online commented Jun 16, 2024

I think that should be done, I really appreciate the assist!

Please let me know if I need to make any additional changes, thank you! :)

EDIT: Crap... I think I made it worse 😑

@emilazy
Copy link
Collaborator

emilazy commented Jun 16, 2024

Looks like you accidentally moved the new upstream commits after yours – if you run git rebase -i 29d16ceb7c13c2741d10f77230caca6163cfa904^ you should be able to just remove all the lines other than your commit and resolve the conflict. Git’s UI is pretty terrible, sadly…

nicknovitski and others added 4 commits June 16, 2024 14:03
Before this commit, aarch64 users building the following configuration
would end up with an aarch64-linux builder, while after it, they get the
x86_64-linux builder they expect:
```nix
 nix.linux-builder = {
  enable = true;
  package = pkgs.darwin.linux-builder-x86_64;
};
```

Before, in order to get an x86_64-linux builder, they would have needed
to use this configuration instead:
```nix
 nix.linux-builder = {
  enable = true;
  config.nixpkgs.hostPlatform = "x86_64-linux";
  systems = ["x86_64-linux"];
};
```

The reason for this is that the linux-builder module calls `override` on
the package option, and the `linux-builder-x86_64` package is also
defined using override:
```nix
linux-builder-x86_64 = linux-builder.override {
  modules = [ { nixpkgs.hostPlatform = "x86_64-linux"; } ];
};
```

The module was effectively discarding the `nixpkgs.hostPlatform` option.

Example issue: NixOS/nixpkgs#313784
checks.nix: disable verifyBuildUsers for auto-allocate-uids
@347Online
Copy link
Author

Sorry for the delayed response - I tried your latest suggestion to no avail, I assume I must not be understanding something? 🤔

I'll keep tinkering

@emilazy
Copy link
Collaborator

emilazy commented Jun 25, 2024

Git’s UI is really awful, it’s not your fault. (I’ll take this opportunity to plug Jujutsu, which makes all of this a lot simpler and easier to recover from; I can’t imagine using Git’s UI directly any more.)

In general when working on Nix ecosystem repositories you almost always want to git pull --rebase and not introduce any merge commits, and liberally use git commit --amend and git rebase --interactive to squash fixes into the original commits. The easiest way to get out of this mess is probably as follows:

# Get the latest revision from LnL7/nix-darwin into FETCH_HEAD
# Replace “upstream” with whatever your https://github.com/LnL7/nix-darwin.git remote is
# (`git remote list` to find out, `git remote add` if you don’t have one)
$ git fetch upstream master

# Put your current branch at FETCH_HEAD without changing your working tree
$ git reset FETCH_HEAD

# Record every change between upstream and your current files as a single commit
$ git commit -a

# Check the commit turned out right; if something looks wrong, adjust your files
# and `git commit --amend -a` until it looks good
$ git show HEAD

# Update your pull request
$ git push --force-with-lease

@347Online
Copy link
Author

Jujutsu sounds VERY cool, I'll have to give it a look

I'm traveling for work this week, but I'll definitely give your suggestions a try when I can.

I really am sorry about all this, I can't tell you how much I appreciate all your help, thank you so much <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants