diff --git a/CHANGELOG.md b/CHANGELOG.md index ddd97e1..446a6f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -73,8 +73,43 @@ mistaken for a safe patch upgrade. `amd64` where linux uses `x86_64`, and macOS uses `arm64` where linux uses `aarch64`. +- `reqtxt`: every `Entry` and `OptionEntry` carries a `Source` with the file and + line it came from, plus `SourceOf` and a `WithPath` parse option. + + `Flatten` splices every include level into one flat `File` and consumes each + `IncludeEntry`, so afterwards nothing remained to attribute an entry to a file, + and no entry carried a line number at all. That matters most for whole-file + options, since `Pre()` and `IndexURL()` are any-wins across the flattened + result: a single `--pre` nested three includes deep, even inside a `-c` + subtree, silently changes what a caller collects. "Enabled by line 2 of + extra.txt" is a materially better diagnostic than "enabled", and on a machine + with no network it is the difference between diagnosable and not. + + `Flatten` sets the path per included file automatically. `Parse` leaves it + empty unless `WithPath` is given, since it takes content rather than a + filename. + ### Fixed +- `reqtxt`: `--all-releases`, `--only-final` and `--use-feature` are recognized. + All three are in pip's `SUPPORTED_OPTIONS`, and the consequence of their + absence was worse than losing normalization: an unrecognized option is assumed + boolean, so its argument was dispatched as its own line and became a + **fabricated requirement**. `--use-feature 2020-resolver` produced a package + named `2020-resolver` from a valid pip file. + + `--all-releases` and `--only-final` are pip's per-package replacements for + `--pre`, which pip refuses to combine with them. + +- `reqtxt`: a standalone `--hash` line is no longer an error. pip logs *"line %s + has --hash but no requirement, and will be ignored"* and continues, so the file + installs fine and rejecting it made this package stricter than pip. It is + surfaced as a file-level option rather than dropped, so a caller can reproduce + pip's warning. + +- `reqtxt`: `Flatten` with a nil `open` callback returns an error instead of + panicking. + - `tags`: `riscv64` and `loongarch64` targets now claim the same manylinux series as every other non-x86 architecture — floored at glibc 2.17, with the `manylinux2014_` legacy alias — matching pypa/packaging. diff --git a/reqtxt/flatten.go b/reqtxt/flatten.go index f506687..5440473 100644 --- a/reqtxt/flatten.go +++ b/reqtxt/flatten.go @@ -41,6 +41,13 @@ var schemeRE = regexp.MustCompile(`^[a-zA-Z][a-zA-Z0-9+.-]*://`) // recursive Parse call, so e.g. WithEnv expansion is applied consistently // at every level of the include tree. func Flatten(root string, open func(path string) ([]byte, error), opts ...ParseOption) (*File, error) { + // open is mandatory and is called for the root before anything else, so a nil + // callback would panic on the first dereference. In a CLI a stack trace is a + // materially worse failure than an error string, and the caller cannot tell + // them apart from the outside. + if open == nil { + return nil, errors.Join(ErrInvalidRequirementsFile, errors.New("Flatten requires a non-nil open callback")) + } entries, err := flattenWalk(root, false, map[string]bool{}, open, opts) if err != nil { return nil, err @@ -70,7 +77,10 @@ func flattenWalk(p string, constraintCtx bool, visited map[string]bool, open fun content := decodeUTF8(raw) - file, err := Parse(content, opts...) + // WithPath is appended rather than prepended so a caller's own WithPath + // cannot silently attribute every included file to one path. Each level + // records the file actually opened. + file, err := Parse(content, append(append([]ParseOption(nil), opts...), WithPath(p))...) if err != nil { return nil, err } diff --git a/reqtxt/parse.go b/reqtxt/parse.go index d0661ad..bb834dd 100644 --- a/reqtxt/parse.go +++ b/reqtxt/parse.go @@ -36,6 +36,15 @@ var knownOptions = map[string]knownOption{ "--no-binary": {optNoBinary, 1}, "--only-binary": {optOnlyBinary, 1}, "--prefer-binary": {optPreferBinary, 0}, + // These three are in pip's SUPPORTED_OPTIONS and were missing here, which + // mattered more than "not normalized": an unknown bare option is assumed + // boolean, so its argument was dispatched as a separate line and became a + // FABRICATED requirement. "--use-feature 2020-resolver" yielded a package + // named "2020-resolver" from a perfectly valid pip file. All three take a + // value (pip: type="str"). + "--all-releases": {optAllReleases, 1}, + "--only-final": {optOnlyFinal, 1}, + "--use-feature": {optUseFeature, 1}, } // perLineValueOptions are the per-requirement options (other than @@ -71,12 +80,42 @@ func Parse(content string, opts ...ParseOption) (*File, error) { if err != nil { return nil, err } + // Stamped here rather than at each construction site: every entry from a + // logical line shares that line, including the per-requirement options + // gathered from its continuations, and one pass cannot miss a site. + src := Source{Path: cfg.path, Line: ll.line} + for _, e := range entries { + stampSource(e, src) + } file.Entries = append(file.Entries, entries...) } return file, nil } +// stampSource records src on e and on any per-requirement options attached to +// it. A continuation line's options report the line the logical line STARTED on, +// which is the same anchor reqtxt already uses for errors raised on a joined +// line. +func stampSource(e Entry, src Source) { + switch n := e.(type) { + case *RequirementEntry: + n.Source = src + for i := range n.Options { + n.Options[i].Source = src + } + case *IncludeEntry: + n.Source = src + case *UnnamedEntry: + n.Source = src + for i := range n.Options { + n.Options[i].Source = src + } + case *OptionEntry: + n.Source = src + } +} + // dispatchLogicalLine routes one preprocessed logical line to the flag // path or the package path, deciding from the line's first // whitespace-delimited raw token - before any shlex tokenization happens. @@ -198,7 +237,28 @@ func dispatchEditable(hasEq bool, eqValue string, rest []string, lineNum int) ([ // only ever a per-requirement option (see attachReqOptions). func dispatchFileOption(name string, hasEq bool, eqValue string, rest []string, lineNum int) ([]Entry, error) { if name == "--hash" { - return nil, lineError(lineNum, "--hash has no associated requirement") + // pip does NOT reject this. req_file.py logs "line %s has --hash but no + // requirement, and will be ignored" and carries on, so a file containing + // one installs fine and rejecting it made this package stricter than the + // thing it models. + // + // Emitted as a file-level OptionEntry rather than dropped, so the + // information survives for a caller that wants to reproduce pip's warning. + // There is nothing to attach it to -- "--hash" is only ever a + // per-requirement option (see attachReqOptions) -- and File's accessors + // ignore names they do not know, so this cannot be mistaken for a + // requirement's hash. + value, remaining, err := takeValue(name, hasEq, eqValue, rest, lineNum) + if err != nil { + // A bare "--hash" with no value at all: still not fatal for pip, which + // never sees a value it can use either way. + value, remaining = eqValue, rest + } + following, err := dispatchLine(remaining, lineNum) + if err != nil { + return nil, err + } + return append([]Entry{&OptionEntry{Name: name, Value: value}}, following...), nil } var ( @@ -228,6 +288,14 @@ func dispatchFileOption(name string, hasEq bool, eqValue string, rest []string, // Unknown flag: "--opt=value" takes the value; a bare "--opt" is // boolean and never consumes the next token — it is left to be // dispatched as its own, separate entry. + // + // That assumption is a guess, and it is the RIGHT guess for a boolean + // followed by a requirement ("--frob foo" yields the option and foo). It is + // the wrong guess for an unknown arity-1 option, where the argument becomes + // a fabricated requirement ("--timeout 60" yields a package named "60"). + // Both readings are guesses and neither is safe, so the fix is not to + // change the guess but to leave fewer options unknown: see knownOptions, + // which is audited against pip's SUPPORTED_OPTIONS. value, remaining = eqValue, rest } diff --git a/reqtxt/parse_test.go b/reqtxt/parse_test.go index b60f9c2..9b89f48 100644 --- a/reqtxt/parse_test.go +++ b/reqtxt/parse_test.go @@ -94,10 +94,10 @@ func TestParse_Include(t *testing.T) { content string want IncludeEntry }{ - {name: "-r space form", content: "-r base.txt", want: IncludeEntry{Path: "base.txt"}}, - {name: "-c space form is a constraint", content: "-c c.txt", want: IncludeEntry{Path: "c.txt", Constraint: true}}, - {name: "--requirement= form", content: "--requirement=x.txt", want: IncludeEntry{Path: "x.txt"}}, - {name: "--constraint= form is a constraint", content: "--constraint=x.txt", want: IncludeEntry{Path: "x.txt", Constraint: true}}, + {name: "-r space form", content: "-r base.txt", want: IncludeEntry{Path: "base.txt", Source: Source{Line: 1}}}, + {name: "-c space form is a constraint", content: "-c c.txt", want: IncludeEntry{Path: "c.txt", Constraint: true, Source: Source{Line: 1}}}, + {name: "--requirement= form", content: "--requirement=x.txt", want: IncludeEntry{Path: "x.txt", Source: Source{Line: 1}}}, + {name: "--constraint= form is a constraint", content: "--constraint=x.txt", want: IncludeEntry{Path: "x.txt", Constraint: true, Source: Source{Line: 1}}}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { @@ -144,27 +144,27 @@ func TestParse_FileOptions(t *testing.T) { { name: "--index-url space form", content: "--index-url https://i/simple", - want: []OptionEntry{{Name: "--index-url", Value: "https://i/simple"}}, + want: []OptionEntry{{Name: "--index-url", Value: "https://i/simple", Source: Source{Line: 1}}}, }, { name: "-i short form normalizes to --index-url", content: "-i https://i/simple", - want: []OptionEntry{{Name: "--index-url", Value: "https://i/simple"}}, + want: []OptionEntry{{Name: "--index-url", Value: "https://i/simple", Source: Source{Line: 1}}}, }, { name: "--index-url= form", content: "--index-url=https://i", - want: []OptionEntry{{Name: "--index-url", Value: "https://i"}}, + want: []OptionEntry{{Name: "--index-url", Value: "https://i", Source: Source{Line: 1}}}, }, { name: "--no-index is boolean", content: "--no-index", - want: []OptionEntry{{Name: "--no-index", Value: ""}}, + want: []OptionEntry{{Name: "--no-index", Value: "", Source: Source{Line: 1}}}, }, { name: "--find-links repeated across lines", content: "--find-links a\n--find-links b", - want: []OptionEntry{{Name: "--find-links", Value: "a"}, {Name: "--find-links", Value: "b"}}, + want: []OptionEntry{{Name: "--find-links", Value: "a", Source: Source{Line: 1}}, {Name: "--find-links", Value: "b", Source: Source{Line: 2}}}, }, } for _, c := range cases { @@ -189,7 +189,7 @@ func TestParse_UnknownFlag(t *testing.T) { oe, ok := f.Entries[0].(*OptionEntry) require.True(t, ok, "want *OptionEntry, got %T", f.Entries[0]) - assert.Equal(t, OptionEntry{Name: "--frobnicate", Value: "1"}, *oe) + assert.Equal(t, OptionEntry{Name: "--frobnicate", Value: "1", Source: Source{Line: 1}}, *oe) f, err = Parse("--frob foo") require.NoError(t, err) @@ -197,7 +197,7 @@ func TestParse_UnknownFlag(t *testing.T) { oe, ok = f.Entries[0].(*OptionEntry) require.True(t, ok, "entry 0: want *OptionEntry, got %T", f.Entries[0]) - assert.Equal(t, OptionEntry{Name: "--frob", Value: ""}, *oe) + assert.Equal(t, OptionEntry{Name: "--frob", Value: "", Source: Source{Line: 1}}, *oe) re, ok := f.Entries[1].(*RequirementEntry) require.True(t, ok, "entry 1: want *RequirementEntry, got %T", f.Entries[1]) @@ -219,9 +219,13 @@ func TestParse_Hashes(t *testing.T) { require.Error(t, err) assert.ErrorIs(t, err, ErrInvalidRequirementsFile) - _, err = Parse("--hash=sha256:x") - require.Error(t, err) - assert.ErrorIs(t, err, ErrInvalidRequirementsFile) + // A standalone --hash is NOT an error: pip logs "line %s has --hash but no + // requirement, and will be ignored" and carries on, so a file containing one + // installs fine. It surfaces as a file-level option instead; see + // TestStandaloneHash_IsNotAnError. + f, err = Parse("--hash=sha256:x") + require.NoError(t, err) + assert.Empty(t, f.Requirements()) } func TestParse_ConfigSettings(t *testing.T) { @@ -232,7 +236,7 @@ func TestParse_ConfigSettings(t *testing.T) { re, ok := f.Entries[0].(*RequirementEntry) require.True(t, ok, "want *RequirementEntry, got %T", f.Entries[0]) require.Len(t, re.Options, 1) - assert.Equal(t, OptionEntry{Name: "--config-settings", Value: "x=y"}, re.Options[0]) + assert.Equal(t, OptionEntry{Name: "--config-settings", Value: "x=y", Source: Source{Line: 1}}, re.Options[0]) } func TestParse_BareArchiveIsUnnamed(t *testing.T) { diff --git a/reqtxt/pipparity_test.go b/reqtxt/pipparity_test.go new file mode 100644 index 0000000..c887058 --- /dev/null +++ b/reqtxt/pipparity_test.go @@ -0,0 +1,130 @@ +// SPDX-License-Identifier: Apache-2.0 OR MIT +package reqtxt + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestKnownOptions_NoFabricatedRequirements is the bug these table entries fix. +// An option missing from knownOptions is assumed boolean, so its argument is +// dispatched as its own line and becomes a requirement. Every one of these is a +// real pip SUPPORTED_OPTIONS entry, so this fired on valid files -- and a +// consumer that resolves the closure of every requirement then hunts for a +// package that does not exist. +func TestKnownOptions_NoFabricatedRequirements(t *testing.T) { + for _, tc := range []struct { + content string + wantName string + wantValue string + }{ + {"--use-feature 2020-resolver", optUseFeature, "2020-resolver"}, + {"--use-feature=2020-resolver", optUseFeature, "2020-resolver"}, + {"--all-releases :all:", optAllReleases, ":all:"}, + {"--all-releases mypkg,other", optAllReleases, "mypkg,other"}, + {"--only-final :all:", optOnlyFinal, ":all:"}, + {"--only-final=mypkg", optOnlyFinal, "mypkg"}, + } { + t.Run(tc.content, func(t *testing.T) { + f, err := Parse(tc.content) + require.NoError(t, err) + + require.Len(t, f.Entries, 1, "must not also produce a requirement") + oe, ok := f.Entries[0].(*OptionEntry) + require.True(t, ok, "want *OptionEntry, got %T", f.Entries[0]) + assert.Equal(t, tc.wantName, oe.Name) + assert.Equal(t, tc.wantValue, oe.Value) + + assert.Empty(t, f.Requirements(), "no requirement may be fabricated") + }) + } +} + +// TestKnownOptions_CoversPipSupportedOptions guards against the same class of gap +// reappearing. Every long option in pip's SUPPORTED_OPTIONS must be handled +// here, either in knownOptions or by a dedicated dispatch path. +func TestKnownOptions_CoversPipSupportedOptions(t *testing.T) { + // pip's SUPPORTED_OPTIONS, by long spelling (pip 26.1, req_file.py). + pipSupported := []string{ + "--index-url", "--extra-index-url", "--no-index", + "--constraint", "--requirement", "--editable", + "--find-links", "--no-binary", "--only-binary", "--prefer-binary", + "--require-hashes", "--pre", "--all-releases", "--only-final", + "--trusted-host", "--use-feature", + } + // These have their own dispatch and never reach knownOptions. + dispatchedElsewhere := map[string]bool{ + "--constraint": true, "--requirement": true, "--editable": true, + } + + for _, opt := range pipSupported { + if dispatchedElsewhere[opt] { + continue + } + _, known := knownOptions[opt] + assert.True(t, known, "%s is in pip's SUPPORTED_OPTIONS but not knownOptions", opt) + } +} + +// TestStandaloneHash_IsNotAnError pins parity with pip, which logs "line %s has +// --hash but no requirement, and will be ignored" (req_file.py) and carries on. +// Rejecting the file made this package stricter than the thing it models. +func TestStandaloneHash_IsNotAnError(t *testing.T) { + for _, content := range []string{ + "--hash=sha256:abc", + "--hash sha256:abc", + "requests==2.0\n--hash=sha256:abc", + } { + t.Run(content, func(t *testing.T) { + f, err := Parse(content) + require.NoError(t, err, "pip accepts this file") + + var found bool + for _, o := range f.Options() { + if o.Name == "--hash" { + found = true + } + } + assert.True(t, found, + "the line is surfaced rather than dropped, so a caller can warn as pip does") + }) + } +} + +// TestStandaloneHash_DoesNotAttachToAPrecedingRequirement guards the thing that +// made this an error in the first place: a file-level --hash must not be mistaken +// for a hash belonging to an earlier requirement. +func TestStandaloneHash_DoesNotAttachToAPrecedingRequirement(t *testing.T) { + f, err := Parse("requests==2.0\n--hash=sha256:abc") + require.NoError(t, err) + + reqs := f.Requirements() + require.Len(t, reqs, 1) + assert.Empty(t, reqs[0].Hashes, "the standalone --hash belongs to no requirement") +} + +// TestFlatten_NilOpenIsAnErrorNotAPanic covers the guard: open is called for the +// root before anything else, so a nil callback used to panic on first deref. In a +// CLI a stack trace is a materially worse failure than an error string. +func TestFlatten_NilOpenIsAnErrorNotAPanic(t *testing.T) { + require.NotPanics(t, func() { + _, err := Flatten("root.txt", nil) + assert.ErrorIs(t, err, ErrInvalidRequirementsFile) + }) +} + +// TestTrailingSemicolonStillRejected records a NON-change. A review claimed pip +// accepts "requests;" where this package rejects it. Checked against packaging +// 26.3: Requirement("requests;") raises InvalidRequirement ("Expected a marker +// variable or quoted string"), so the rejection is faithful and stays. +func TestTrailingSemicolonStillRejected(t *testing.T) { + for _, content := range []string{"requests;", "requests ;"} { + t.Run(content, func(t *testing.T) { + _, err := Parse(content) + assert.ErrorIs(t, err, ErrInvalidRequirementsFile, + "packaging raises InvalidRequirement here too") + }) + } +} diff --git a/reqtxt/preprocess.go b/reqtxt/preprocess.go index 942566e..6b98562 100644 --- a/reqtxt/preprocess.go +++ b/reqtxt/preprocess.go @@ -54,6 +54,19 @@ type parseConfig struct { // and whether it is set. When nil, ${VAR} references are left // literal (no expansion is attempted). env func(string) (string, bool) + // path is recorded on every entry's Source. Empty unless WithPath was + // given; Flatten supplies it per included file. + path string +} + +// WithPath records path on the Source of every entry Parse produces. Parse takes +// content rather than a filename, so it cannot know where that content came +// from; without this a caller cannot attribute an entry to a file. +// +// Flatten sets this per included file automatically, so provenance survives the +// flattening that consumes each IncludeEntry. +func WithPath(path string) ParseOption { + return func(cfg *parseConfig) { cfg.path = path } } // WithEnv enables "${VAR}" expansion using lookup (e.g. os.LookupEnv). diff --git a/reqtxt/source_test.go b/reqtxt/source_test.go new file mode 100644 index 0000000..7a918db --- /dev/null +++ b/reqtxt/source_test.go @@ -0,0 +1,168 @@ +// SPDX-License-Identifier: Apache-2.0 OR MIT +package reqtxt + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// openMap serves a fixed set of files, so include trees are testable without +// touching disk. +func openMap(files map[string]string) func(string) ([]byte, error) { + return func(path string) ([]byte, error) { + content, ok := files[path] + if !ok { + return nil, fmt.Errorf("no such file: %s", path) + } + return []byte(content), nil + } +} + +func TestSource_LineNumbers(t *testing.T) { + f, err := Parse("--no-index\nrequests==2.0\n\n-r base.txt\n./local/pkg") + require.NoError(t, err) + require.Len(t, f.Entries, 4) + + for i, wantLine := range []int{1, 2, 4, 5} { + assert.Equal(t, wantLine, SourceOf(f.Entries[i]).Line, "entry %d", i) + } +} + +// TestSource_PathEmptyWithoutWithPath documents the default: Parse takes content, +// not a filename, so it cannot invent a path. +func TestSource_PathEmptyWithoutWithPath(t *testing.T) { + f, err := Parse("requests==2.0") + require.NoError(t, err) + require.Len(t, f.Entries, 1) + assert.Empty(t, SourceOf(f.Entries[0]).Path) + + f, err = Parse("requests==2.0", WithPath("given.txt")) + require.NoError(t, err) + assert.Equal(t, "given.txt", SourceOf(f.Entries[0]).Path) +} + +// TestSource_FlattenAttributesEachFile is the case the feature exists for: after +// flattening, every entry still names the file it came from, even though the +// IncludeEntry that pulled it in has been consumed. +func TestSource_FlattenAttributesEachFile(t *testing.T) { + f, err := Flatten("root.txt", openMap(map[string]string{ + "root.txt": "requests==2.0\n-r base.txt\nflask==3.0", + "base.txt": "urllib3==2.0\n-r deep.txt", + "deep.txt": "six==1.16", + })) + require.NoError(t, err) + + got := map[string]Source{} + for _, e := range f.Entries { + re, ok := e.(*RequirementEntry) + require.True(t, ok, "want only requirements, got %T", e) + got[re.Requirement.Name] = re.Source + } + + assert.Equal(t, Source{Path: "root.txt", Line: 1}, got["requests"]) + assert.Equal(t, Source{Path: "base.txt", Line: 1}, got["urllib3"]) + assert.Equal(t, Source{Path: "deep.txt", Line: 1}, got["six"]) + assert.Equal(t, Source{Path: "root.txt", Line: 3}, got["flask"]) + + // No IncludeEntry survives flattening, so Source is the only provenance left. + assert.Empty(t, f.Includes()) +} + +// TestSource_PreLeaksFromAConstraintsFile is the diagnostic that motivated this. +// File.Pre() is any-wins across the flattened result, so a single "--pre" nested +// inside a "-c" subtree silently changes what a caller collects. Source is what +// lets the caller say WHERE it came from. +func TestSource_PreLeaksFromAConstraintsFile(t *testing.T) { + f, err := Flatten("root.txt", openMap(map[string]string{ + "root.txt": "requests==2.0\n-c pins.txt", + "pins.txt": "urllib3<2\n-r extra.txt", + "extra.txt": "# harmless looking\n--pre\nsix==1.16", + })) + require.NoError(t, err) + + require.True(t, f.Pre(), "--pre anywhere in the tree flips it") + + // And now it is attributable. + var where []Source + for _, o := range f.Options() { + if o.Name == optPre { + where = append(where, o.Source) + } + } + require.Len(t, where, 1) + assert.Equal(t, Source{Path: "extra.txt", Line: 2}, where[0], + "line 2 because line 1 of extra.txt is a comment") +} + +// TestSource_ConstraintPromotionIsAttributable covers the other warning a +// consumer wants: a "-r" nested inside a "-c" file resets constraint-ness, so +// those pins become requirements. Source names the file responsible. +func TestSource_ConstraintPromotionIsAttributable(t *testing.T) { + f, err := Flatten("root.txt", openMap(map[string]string{ + "root.txt": "-c pins.txt", + "pins.txt": "urllib3<2\n-r promoted.txt", + "promoted.txt": "six==1.16", + })) + require.NoError(t, err) + + byName := map[string]*RequirementEntry{} + for _, e := range f.Entries { + if re, ok := e.(*RequirementEntry); ok { + byName[re.Requirement.Name] = re + } + } + + require.Contains(t, byName, "urllib3") + assert.True(t, byName["urllib3"].Constraint, "reached directly via -c") + assert.Equal(t, "pins.txt", byName["urllib3"].Source.Path) + + require.Contains(t, byName, "six") + assert.False(t, byName["six"].Constraint, + "a -r inside a -c resets constraint-ness, matching pip") + assert.Equal(t, "promoted.txt", byName["six"].Source.Path, + "and Source names the file that did it") +} + +// TestSource_PerRequirementOptions pins that options gathered from continuation +// lines are attributed to the logical line their requirement started on. +func TestSource_PerRequirementOptions(t *testing.T) { + f, err := Parse("# leading comment\nfoo==1.0 \\\n --config-settings=x=y", WithPath("r.txt")) + require.NoError(t, err) + require.Len(t, f.Entries, 1) + + re, ok := f.Entries[0].(*RequirementEntry) + require.True(t, ok) + require.Len(t, re.Options, 1) + + assert.Equal(t, Source{Path: "r.txt", Line: 2}, re.Source) + assert.Equal(t, re.Source, re.Options[0].Source, + "a continuation's option shares its requirement's anchor") +} + +// TestSource_CallerWithPathDoesNotOverrideFlatten guards the append order: a +// caller-supplied WithPath must not attribute every included file to one path. +func TestSource_CallerWithPathDoesNotOverrideFlatten(t *testing.T) { + f, err := Flatten("root.txt", openMap(map[string]string{ + "root.txt": "-r base.txt", + "base.txt": "six==1.16", + }), WithPath("caller-supplied.txt")) + require.NoError(t, err) + require.Len(t, f.Entries, 1) + + assert.Equal(t, "base.txt", SourceOf(f.Entries[0]).Path) +} + +func TestSourceOf_CoversEveryEntryType(t *testing.T) { + f, err := Parse("--no-index\nrequests==2.0\n-r base.txt\n./local/pkg", WithPath("r.txt")) + require.NoError(t, err) + require.Len(t, f.Entries, 4) + + for i, e := range f.Entries { + src := SourceOf(e) + assert.Equal(t, "r.txt", src.Path, "entry %d (%T)", i, e) + assert.NotZero(t, src.Line, "entry %d (%T)", i, e) + } +} diff --git a/reqtxt/types.go b/reqtxt/types.go index b034b93..3ab8848 100644 --- a/reqtxt/types.go +++ b/reqtxt/types.go @@ -18,6 +18,44 @@ type File struct { Entries []Entry } +// Source records where an entry came from. After Flatten, Path is the include +// target as opened, so an entry's origin survives the flattening that consumes +// and discards the IncludeEntry that pulled it in. +// +// Without this, a caller cannot report which file supplied a given entry or +// option, and after Flatten there is nothing left to deduce it from. That +// matters most for whole-file options: "--pre" is any-wins across the flattened +// result, so a single "--pre" nested three includes deep silently changes what a +// caller collects, and "changed by line 4 of shared-constraints.txt" is a +// materially different diagnostic from "changed". +// +// Path is empty after a bare Parse unless WithPath was given; Line is 1-based +// and is 0 only where no line applies. +type Source struct { + Path string + Line int +} + +// SourceOf returns the Source of any Entry. It exists so a caller iterating +// File.Entries for diagnostics does not need a type switch purely to read +// provenance. +func SourceOf(e Entry) Source { + switch n := e.(type) { + case *RequirementEntry: + return n.Source + case *IncludeEntry: + return n.Source + case *UnnamedEntry: + return n.Source + case *OptionEntry: + return n.Source + default: + // Unreachable: Entry's entry() method is unexported, so the set of + // implementations is closed to this package. + return Source{} + } +} + // Entry is one line's worth of parsed requirements.txt content. It is // implemented by *RequirementEntry, *IncludeEntry, *UnnamedEntry, and // *OptionEntry. @@ -38,6 +76,8 @@ type RequirementEntry struct { // inherited through nested includes, so a "-r" nested inside a "-c" file // yields non-constraint entries. Set by Flatten; false after Parse alone. Constraint bool + // Source records the file and line this entry came from. + Source Source } func (*RequirementEntry) entry() {} @@ -49,6 +89,8 @@ type IncludeEntry struct { // Constraint is true if this include used "-c"/"--constraint" rather // than "-r"/"--requirement". Constraint bool + // Source records the file and line this entry came from. + Source Source } func (*IncludeEntry) entry() {} @@ -71,6 +113,8 @@ type UnnamedEntry struct { // inherited through nested includes, so a "-r" nested inside a "-c" file // yields non-constraint entries. Set by Flatten; false after Parse alone. Constraint bool + // Source records the file and line this entry came from. + Source Source } func (*UnnamedEntry) entry() {} @@ -81,6 +125,8 @@ func (*UnnamedEntry) entry() {} type OptionEntry struct { Name string Value string + // Source records the file and line this entry came from. + Source Source } func (*OptionEntry) entry() {} @@ -132,6 +178,13 @@ const ( optNoBinary = "--no-binary" optOnlyBinary = "--only-binary" optPreferBinary = "--prefer-binary" + // optAllReleases and optOnlyFinal are pip's per-package replacements for + // --pre, which pip rejects in combination with it. Both take a value: + // ":all:", ":none:", or a comma-separated package list. + optAllReleases = "--all-releases" + optOnlyFinal = "--only-final" + // optUseFeature is pip's --use-feature . + optUseFeature = "--use-feature" ) // Requirements returns the RequirementEntry values in f.Entries, in file