add nix package manager by reshmifrog · Pull Request #442 · jfrog/jfrog-cli-artifactory

reshmifrog · 2026-05-07T04:55:16Z

All tests passed. If this feature is not already covered by the tests, I added new tests.
All static analysis checks passed.
Appropriate label is added to auto generate release notes.
I used gofmt for formatting the code before submitting the pull request.
PR description is clear and concise, and it includes the proposed solution/fix.

Added the support for nix package manager

…g fixes

Balasubramanyamkosuri

Review summary

Solid NixCommand implementation — clear dispatch by native tool and appropriate netrc auth with cleanup.

Suggestions (non-blocking)

Document which subcommands collect build-info vs passthrough (nix-channel is passthrough only).
Add an IT or doc example for nix copy URL format matching Artifactory Nix repo Set Me Up (api/nix/<repo>/).
Clear error when nix / nix-env / nix-channel is not on PATH.
After build-info-go#378 merges (with mergeArtifacts fix), pin released build-info-go version in go.mod.

Blocked on: build-info-go#378 mergeArtifacts fix before final dependency bump.

Structured review: docs/reviews/nix-pr-review.md in team workspace.

naveenku-jfrog · 2026-05-21T08:10:15Z

+	tmpFile, err := os.CreateTemp("", "nix-netrc-")
+	if err != nil {
+		return
+	}
+	if _, err = tmpFile.WriteString(netrcContent); err != nil {
+		return
+	}
+	if err = tmpFile.Close(); err != nil {
+		return
+	}
+	c.netrcPath = tmpFile.Name()


Two issues here:

If WriteString (line 610) or Close (line 613) fails, the function
returns early but tmpFile is never closed and the temp file is never
deleted — OS fd + disk leak. Add _ = tmpFile.Close(); _ = os.Remove(tmpFile.Name())
in each early-return error path.

The function has no return value, so auth failures are invisible to
the caller. Callers in Run() (line 80) just continue silently and Nix
runs unauthenticated, producing cryptic "access denied" errors downstream.
Change the signature to createNetrcFile() error and return fmt.Errorf
on each failure.

naveenku-jfrog · 2026-05-21T08:11:41Z

+	req, err := http.NewRequest(http.MethodGet, url, nil)
+	if err != nil {
+		return ""
+	}
+	if c.serverDetails.User != "" {
+		password := c.serverDetails.Password
+		if password == "" {
+			password = c.serverDetails.AccessToken
+		}
+		req.SetBasicAuth(c.serverDetails.User, password)
+	}
+
+	resp, err := client.Do(req)
+	if err != nil || resp.StatusCode != http.StatusOK {
+		if resp != nil {
+			_ = resp.Body.Close()
+		}
+		return ""
+	}
+	defer func() { _ = resp.Body.Close() }()
+
+	var repoConfig struct {
+		Rclass                string `json:"rclass"`
+		DefaultDeploymentRepo string `json:"defaultDeploymentRepo"`
+	}
+	if err := json.NewDecoder(resp.Body).Decode(&repoConfig); err != nil {
+		return ""
+	}
+
+	if repoConfig.Rclass == "virtual" && repoConfig.DefaultDeploymentRepo != "" {
+		return repoConfig.DefaultDeploymentRepo
+	}
+	return ""


This raw http.Client bypasses all the jfrog-client-go transport layer, which means:

InsecureTls / custom CA certs in ServerDetails are ignored.

Token-only auth is silently skipped — line 489 checks c.serverDetails.User != ""
but modern JFrog Platform setups often use access tokens with no User field,
so no Authorization header is set at all.
Fix: if User is empty but AccessToken is set, use:
req.Header.Set("Authorization", "Bearer "+c.serverDetails.AccessToken)

Consider replacing this with the existing Artifactory service manager /
jfrog-client-go repository API to get retries and proper TLS handling for free.

naveenku-jfrog · 2026-05-21T08:12:17Z

+				narXzName, narXzPath := c.findNarFile(searchRepo, dirPath)
+				if narXzName != "" {
+					artChecksum := c.getArtifactChecksums(searchRepo, narXzPath)
+					if artChecksum.Sha1 != "" || artChecksum.Sha256 != "" {
+						buildInfo.Modules[0].Dependencies[i].Checksum = artChecksum
+						resolved++
+					}
+				}
+			}


findNarFile() (line 327/398) and getArtifactChecksums() (line 329/413)
each call utils.CreateServiceManager internally. For a Nix closure with
50–200 store paths this creates hundreds of ServiceManager instances and
HTTP connection pools in a tight loop.
Create a single ServiceManager once before the loop and thread it through,
or promote it to a field on NixCommand set during Run().

naveenku-jfrog · 2026-05-21T08:12:45Z

+	var pkgAttr string
+	for i := len(c.args) - 1; i >= 0; i-- {
+		if !strings.HasPrefix(c.args[i], "-") && strings.Contains(c.args[i], ".") {
+			pkgAttr = c.args[i]
+			break
+		}
+	}
+	if pkgAttr == "" {
+		return nil
+	}


The dot-check on line 248 means nix-env -i hello silently skips
build-info collection with no warning. Package names without a channel
prefix (e.g. plain "hello", "git") are completely ignored.
At minimum add a log.Warn here so the user knows why no build-info
was collected. Ideally document in the method comment which invocation
patterns are supported.

naveenku-jfrog · 2026-05-21T08:13:47Z

+	output, err := cmd.Output()
+	if err != nil {
+		return fmt.Errorf("nix-build failed: %s: %w", string(output), err)


cmd.Output() captures stdout; nix-build writes store paths to stdout,
not errors. Errors go to stderr which is already forwarded to os.Stderr
(line 140), so string(output) in the error message will be the store
path (or empty), not a diagnostic.
Use cmd.CombinedOutput() or just drop string(output) from the
fmt.Errorf call.

naveenku-jfrog · 2026-05-21T08:14:43Z

+func (c *NixCommand) setBuildPropertiesInDir(repo, dirPath, namePattern, buildName, buildNumber, timestamp string) {
+	if c.serverDetails == nil {
+		return
+	}
+	servicesManager, err := utils.CreateServiceManager(c.serverDetails, -1, 0, false)
+	if err != nil {
+		return
+	}
+
+	props := fmt.Sprintf("build.name=%s;build.number=%s;build.timestamp=%s", buildName, buildNumber, timestamp)
+	searchQuery := fmt.Sprintf(`{"repo": "%s", "$or": [{"$and":[{"path": "%s","name": {"$match": "%s"}}]}]}`, repo, dirPath, namePattern)
+	searchParams := services.SearchParams{
+		CommonParams: &specutils.CommonParams{
+			Aql: specutils.Aql{ItemsFind: searchQuery},
+		},
+	}
+	reader, err := servicesManager.SearchFiles(searchParams)
+	if err != nil {
+		return
+	}
+	_, _ = servicesManager.SetProps(services.PropsParams{Reader: reader, Props: props})
+}


Every error path in this function returns without logging or surfacing
anything. A failed SetProps call is completely invisible to the user.
The function should at minimum log.Warn on each failure, and ideally
return error so the caller (tagUploadedArtifacts) can decide whether
to abort or continue.

fluxxBot · 2026-05-24T17:28:43Z

+	case "nix-channel":
+		return c.runNixChannel()
+	case "nix-env":
+		return c.runNixEnv()
+	case "nix-build":
+		return c.runNixBuild()
+	case "build":
+		return c.runNixFlakeBuild()
+	case "copy":
+		return c.runNixCopy()


do we actually need to capture all this commands? what about nixEnv, will capturing this modify our build-info?

fluxxBot · 2026-05-24T17:29:58Z

+func (c *NixCommand) runNixChannel() error {
+	log.Info("Running nix-channel")
+	cmd := exec.Command("nix-channel", c.args...)
+	cmd.Env = c.buildEnv()
+	cmd.Stdout = os.Stdout
+	cmd.Stderr = os.Stderr
+	if err := cmd.Run(); err != nil {
+		return fmt.Errorf("nix-channel failed: %w", err)
+	}
+	return nil
+}


why can't this be a simple pass through?

fluxxBot · 2026-05-24T17:30:52Z

+	log.Info("Running nix-env")
+	cmd := exec.Command("nix-env", c.args...)
+	cmd.Env = c.buildEnv()
+	cmd.Stdout = os.Stdout
+	cmd.Stderr = os.Stderr
+	if err := cmd.Run(); err != nil {
+		return fmt.Errorf("nix-env failed: %w", err)
+	}


this line is being constantly repeated, consider making this a utility function or a mere pass through

fluxxBot · 2026-05-24T17:33:38Z

+	if c.repo != "" && c.serverDetails != nil {
+		deployRepo := c.resolveDefaultDeploymentRepo(c.repo)
+		if deployRepo != "" && deployRepo != c.repo {
+			log.Info(fmt.Sprintf("Resolved default deployment repo: %s → %s", c.repo, deployRepo))
+			c.replaceRepoInToArg(c.repo, deployRepo)
+			c.repo = deployRepo
+			// Add --refresh so nix re-checks the LOCAL repo (which is empty)
+			// instead of using cached knowledge from previous virtual repo checks
+			c.args = append([]string{"--refresh"}, c.args...)
+		}
+	}


check if a similar utility method exists, also what happens if defaultdDeploymentRepo is not present

fluxxBot · 2026-05-24T17:35:04Z

+	return nil
+}
+
+// collectDepsFromEnvArgs resolves the store path from nix-env args (e.g., "nixpkgs.hello").


why do we have so many different ways and logic across build-info-go and here to resolve storePaths?

fluxxBot · 2026-05-24T17:39:15Z

+	return entities.Checksum{}
+}
+
+func (c *NixCommand) getBuildNameAndNumber() (string, string, error) {


a common utility might also be present, also are we not collecting project info from flag?

fluxxBot · 2026-05-24T17:40:17Z

+}
+
+// getArtifactChecksums fetches sha1/sha256/md5 for a file in Artifactory.
+func (c *NixCommand) getArtifactChecksums(repo, pathInRepo string) entities.Checksum {


why are we making a call per file basis? let's group them per repo and make a single shot call to get all the details

fluxxBot · 2026-05-24T17:42:09Z

+	}
+
+	props := fmt.Sprintf("build.name=%s;build.number=%s;build.timestamp=%s", buildName, buildNumber, timestamp)
+	searchQuery := fmt.Sprintf(`{"repo": "%s", "$or": [{"$and":[{"path": "%s","name": {"$match": "%s"}}]}]}`, repo, dirPath, namePattern)


https://github.com/jfrog/jfrog-cli-artifactory/pull/442/changes#diff-9ef932742f174167bd4f2969755f537daa50c6562bacad7cdd71b66ee31701d9R653
findNarFiles function might already be giving you this info, consider caching it and reusing to reduce network calls

fluxxBot · 2026-05-24T17:42:50Z

+}
+
+// createNetrcFile creates a temporary netrc file for Nix authentication.
+func (c *NixCommand) createNetrcFile() {


why are we doing this? are we not considering user's config files?

fluxxBot · 2026-05-24T17:44:17Z

+	timestamp := strconv.FormatInt(time.Now().UnixMilli(), 10)
+	var artifacts []entities.Artifact
+
+	for _, sp := range closurePaths {


let's try to combine them in a single network call, check for better efficiency

add nix package manager

c252545

reshmifrog requested review from a team, agrasth, bhanurp, fluxxBot, itsmeleela and udaykb2 May 7, 2026 04:55

reshmifrog added the new feature Automatically generated release notes label May 7, 2026

reshmifrog requested a deployment to frogbot May 7, 2026 04:55 — with GitHub Actions Waiting

reshmifrog added the safe to test Approve running integration tests on a pull request label May 7, 2026

github-actions Bot removed the safe to test Approve running integration tests on a pull request label May 7, 2026

update nix adapter with --refresh, build.timestamp, parseRepoFromToAr…

1a22779

…g fixes

reshmifrog requested a deployment to frogbot May 7, 2026 05:59 — with GitHub Actions Waiting

update deps

fb9487e

reshmifrog requested a deployment to frogbot May 18, 2026 04:02 — with GitHub Actions Waiting

Merge branch 'main' into RTECO-1040-Nix-package-support

50fc71e

reshmifrog requested a deployment to frogbot May 18, 2026 05:33 — with GitHub Actions Waiting

fix static check and lints

1ad0104

reshmifrog requested a deployment to frogbot May 18, 2026 06:10 — with GitHub Actions Waiting

resolve lints

57c1e4d

reshmifrog requested a deployment to frogbot May 18, 2026 06:19 — with GitHub Actions Waiting

Balasubramanyamkosuri approved these changes May 19, 2026

View reviewed changes

agrasth reviewed May 19, 2026

View reviewed changes

naveenku-jfrog requested changes May 21, 2026

View reviewed changes

fluxxBot requested changes May 24, 2026

View reviewed changes

Conversation

reshmifrog commented May 7, 2026

Uh oh!

Balasubramanyamkosuri left a comment

Choose a reason for hiding this comment

Review summary

Suggestions (non-blocking)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants