Skip to content

Conversation

savil
Copy link
Collaborator

@savil savil commented Feb 8, 2023

Summary

This PR adds the following top-level functions:

  • devbox.addPackagesToProfile: used during devbox add and devbox shell.
  • devbox.removePackagesFromProfile: used during devbox rm.

This PR's size comes from inspecting the results of nix profile list, which we parse into nix.ProfileListItem structs. We use this information in two ways:

  1. In devbox.addPackagesToProfile, we inspect list of nix.ProfileListItem to determine which of the devbox.json packages are pending installation.
  2. In devbox.removePackagesFromProfile, we need the "attribute path" of the package, which we derive from its nix.ProfileListItem

UX:
The UX can be improved, but I didn't invest in it since the PR size is already large. That can be a discussion and a follow up PR.

TODOs for future PRs:

  • Ensure the github:NixOS/nixpkgs/<commit> URL can leverage nixed cache service
  • During devbox add we always list nixpkgs as being installed, which is kinda odd. Can we check that it has been pre-fetched already?
  • UX improvements
  • Older non-flakes profiles link to bin, lib, share, but with flakes profiles we just link to bin and share

How was it tested?

>  DEVBOX_FEATURE_FLAKES=1 DEVBOX_FEATURE_NIXLESS_SHELL=1 DEVBOX_DEBUG=0 devbox shell
(devbox)>  DEVBOX_FEATURE_FLAKES=1 DEVBOX_FEATURE_NIXLESS_SHELL=1 DEVBOX_DEBUG=0 devbox add vim emacs
(devbox)>  DEVBOX_FEATURE_FLAKES=1 DEVBOX_FEATURE_NIXLESS_SHELL=1 DEVBOX_DEBUG=0 devbox rm emacs

Installing looks like below. I did not use the stepper component so that we show the output of each individual install.

Screenshot 2023-02-08 at 5 17 39 PM

Copy link
Collaborator Author

savil commented Feb 8, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@savil savil force-pushed the savil/flake-install branch 5 times, most recently from dc76cc9 to bb0f540 Compare February 8, 2023 05:46
@savil savil force-pushed the savil/flake-install branch from bb0f540 to d8bbd2c Compare February 8, 2023 06:06
savil added a commit that referenced this pull request Feb 8, 2023
## Summary

These functions are no longer going to be used. Superseded by #597

## How was it tested?

compiles
@savil savil force-pushed the savil/flakes-print-dev-env-shell branch from 1681324 to 2da5340 Compare February 8, 2023 17:25
@savil savil force-pushed the savil/flake-install branch from d8bbd2c to edf3a68 Compare February 8, 2023 17:25
Base automatically changed from savil/flakes-print-dev-env-shell to main February 8, 2023 17:36
@savil savil force-pushed the savil/flake-install branch from edf3a68 to 9cf1a8a Compare February 8, 2023 17:39
savil added a commit that referenced this pull request Feb 8, 2023
## Summary

In #597, I wanted to introduce a file called `packages.go` (currently,
called install.go but that's a poor substitute IMO).
This file `pkgs.go` would be confusing to have at the same time.

Since it is unused, lets remove it. It can be brought back whenever
needed.

## How was it tested?

compiles
@savil savil force-pushed the savil/flake-install branch 10 times, most recently from e03c0f6 to ef5c19b Compare February 9, 2023 01:36
@savil savil marked this pull request as ready for review February 9, 2023 01:37
@savil savil force-pushed the savil/flake-install branch from ef5c19b to 86cbc13 Compare February 9, 2023 01:42
}

// nixProfileListItems returns a list of the installed packages
func (d *Devbox) nixProfileListItems() ([]*nixProfileListItem, error) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this and below code is better moved to nix/profile.go so it'll be nix.ProfileListItems()

Copy link
Collaborator

@gcurtis gcurtis left a comment

Choose a reason for hiding this comment

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

I think package name collisions are the only must-fix thing. Is there a performance difference in invoking Nix for each package vs. doing them all at once?

// Using an example:
// 0 github:NixOS/nixpkgs/52e3e80afff4b16ccb7c52e9f0f5220552f03d04#legacyPackages.x86_64-darwin.go_1_19 github:NixOS/nixpkgs/52e3e80afff4b16ccb7c52e9f0f5220552f03d04#legacyPackages.x86_64-darwin.go_1_19 /nix/store/w0lyimyyxxfl3gw40n46rpn1yjrl3q85-go-1.19.3
// 1 github:NixOS/nixpkgs/52e3e80afff4b16ccb7c52e9f0f5220552f03d04#legacyPackages.x86_64-darwin.vim github:NixOS/nixpkgs/52e3e80afff4b16ccb7c52e9f0f5220552f03d04#legacyPackages.x86_64-darwin.vim /nix/store/gapbqxx1d49077jk8ay38z11wgr12p23-vim-9.0.0609
lines := strings.Split(strings.TrimSpace(string(out)), "\n")
Copy link
Collaborator

Choose a reason for hiding this comment

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

A potential way to simplify the parsing is to use a bufio.Scanner. For example:

out, err := cmd.StdoutPipe()
scanner := bufio.NewScanner(out)
scanner.Split(bufio.ScanWords)
for scanner.Scan() {
    index, err := strconv.Atoi(scanner.Text())
    if !scanner.Scan() {
        return errors.New("incomplete nix profile list line")
    }
    unlockedReference := scanner.Text()
    if !scanner.Scan() {
        return errors.New("incomplete nix profile list line")
    }
    lockedReference := scanner.Text()
    if !scanner.Scan() {
        return errors.New("incomplete nix profile list line")
    }
    nixStorePath := scanner.Text()

    items = append(items, ...)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Much nicer now, thanks for the tip to use scanner (and helpful code).

)
}

packageName := parts[len(parts)-1]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This won't necessarily be unique. For example, python310Packages.numpy and python311Packages.numpy will both become numpy. I think that'll affect the maps that use the package name as a key.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point.

I could parse it the other direction assuming the prefix will always be legacyPackages.<platform>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a test case and updated code to fix this.

@savil savil force-pushed the savil/flake-install branch from 14886d2 to 5e1f3b0 Compare February 9, 2023 22:54
@savil savil merged commit 517938f into main Feb 9, 2023
@savil savil deleted the savil/flake-install branch February 9, 2023 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants