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.

Sign up for GitHub

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

Jump to bottom

feat(internal/godocfx): add support for other modules #4290

Merged
merged 6 commits into from Jun 22, 2021
Merged

feat(internal/godocfx): add support for other modules #4290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c2aa309
feat(internal/godocfx): add support for other modules
tbpg Jun 21, 2021
fd89a07
tests: fix them
tbpg Jun 21, 2021
8e1266e
Merge branch 'master' into godocfx
tbpg Jun 21, 2021
37b6704
Merge branch 'master' into godocfx
tbpg Jun 22, 2021
c137e7e
Merge branch 'master' into godocfx
gcf-merge-on-green[bot] Jun 22, 2021
fb0628e
Merge branch 'master' into godocfx
tbpg Jun 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 5 additions & 5 deletions internal/godocfx/index.go
View file
Open in desktop
Expand Up @@ -28,7 +28,7 @@ import (

// indexer gets a limited list of entries from index.golang.org.
type indexer interface {
get(prefix string, since time.Time) (entries []indexEntry, last time.Time, err error)
get(prefixes []string, since time.Time) (entries []indexEntry, last time.Time, err error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are overlapping prefixes an issue that may need consideration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I don't think so. We won't process duplicate modules because we don't process each prefix independently -- if a module matches any prefix, it's included.

}

// indexClient is used to access index.golang.org.
Expand All @@ -49,7 +49,7 @@ type indexEntry struct {
// versions since the given timestamp.
//
// newModules stores the timestamp of the last successful run with tSaver.
func newModules(ctx context.Context, i indexer, tSaver timeSaver, prefix string) ([]indexEntry, error) {
func newModules(ctx context.Context, i indexer, tSaver timeSaver, prefixes []string) ([]indexEntry, error) {
since, err := tSaver.get(ctx)
if err != nil {
return nil, err
Expand All @@ -61,7 +61,7 @@ func newModules(ctx context.Context, i indexer, tSaver timeSaver, prefix string)
for {
count++
var cur []indexEntry
cur, since, err = i.get(prefix, since)
cur, since, err = i.get(prefixes, since)
if err != nil {
return nil, err
}
Expand All @@ -80,7 +80,7 @@ func newModules(ctx context.Context, i indexer, tSaver timeSaver, prefix string)

// get fetches a single chronological page of modules from
// index.golang.org/index.
func (indexClient) get(prefix string, since time.Time) ([]indexEntry, time.Time, error) {
func (indexClient) get(prefixes []string, since time.Time) ([]indexEntry, time.Time, error) {
entries := []indexEntry{}
sinceString := since.Format(time.RFC3339)
resp, err := http.Get("https://index.golang.org/index?since=" + sinceString)
Expand All @@ -96,7 +96,7 @@ func (indexClient) get(prefix string, since time.Time) ([]indexEntry, time.Time,
return nil, time.Time{}, err
}
last = e.Timestamp // Always update the last timestamp.
if !strings.HasPrefix(e.Path, prefix) ||
if !hasPrefix(e.Path, prefixes) ||
strings.Contains(e.Path, "internal") ||
strings.Contains(e.Path, "third_party") ||
strings.Contains(e.Version, "-") { // Filter out pseudo-versions.
Expand Down
4 changes: 2 additions & 2 deletions internal/godocfx/index_test.go
View file
Open in desktop
Expand Up @@ -26,7 +26,7 @@ const wantEntries = 5

type fakeIC struct{}

func (f fakeIC) get(prefix string, since time.Time) (entries []indexEntry, last time.Time, err error) {
func (f fakeIC) get(prefixes []string, since time.Time) (entries []indexEntry, last time.Time, err error) {
e := indexEntry{Timestamp: since.Add(24 * time.Hour)}
return []indexEntry{e}, e.Timestamp, nil
}
Expand All @@ -49,7 +49,7 @@ func (f *fakeTS) put(context.Context, time.Time) error {
func TestNewModules(t *testing.T) {
ic := fakeIC{}
ts := &fakeTS{}
entries, err := newModules(context.Background(), ic, ts, "cloud.google.com")
entries, err := newModules(context.Background(), ic, ts, []string{"cloud.google.com"})
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe include an entry with explicit versioning and maybe something that exercises the filter paths?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test really just makes sure the paging logic works for the index. It doesn't exercise the prefix logic, which is a gap in coverage. Can try to figure out how to add a test for it (need to refactor the HTTP calls) if you feel strongly.

if err != nil {
t.Fatalf("newModules got err: %v", err)
}
Expand Down
62 changes: 31 additions & 31 deletions internal/godocfx/main.go
View file
Open in desktop
Expand Up @@ -58,42 +58,43 @@ func main() {
outDir := flag.String("out", "obj/api", "Output directory (default obj/api)")
projectID := flag.String("project", "", "Project ID to use. Required when using -new-modules.")
newMods := flag.Bool("new-modules", false, "Process all new modules with the given prefix. Uses timestamp in Datastore. Stores results in $out/$mod.")
// TODO: flag to set output URL path

log.SetPrefix("[godocfx] ")

flag.Parse()
if flag.NArg() != 1 {
if flag.NArg() == 0 {
log.Fatalf("%s missing required argument: module path/prefix", os.Args[0])
}

mod := flag.Arg(0)
modNames := flag.Args()
var mods []indexEntry
if *newMods {
if *projectID == "" {
log.Fatal("Must set -project when using -new-modules")
}
var err error
mods, err = newModules(context.Background(), indexClient{}, &dsTimeSaver{projectID: *projectID}, mod)
mods, err = newModules(context.Background(), indexClient{}, &dsTimeSaver{projectID: *projectID}, modNames)
if err != nil {
log.Fatal(err)
}
} else {
modPath := mod
version := "latest"
if strings.Contains(mod, "@") {
parts := strings.Split(mod, "@")
if len(parts) != 2 {
log.Fatal("module arg expected only one '@'")
for _, mod := range modNames {
modPath := mod
version := "latest"
if strings.Contains(mod, "@") {
parts := strings.Split(mod, "@")
if len(parts) != 2 {
log.Fatal("module arg expected only one '@'")
}
modPath = parts[0]
version = parts[1]
}
modPath = parts[0]
version = parts[1]
}
modPath = strings.TrimSuffix(modPath, "/...") // No /... needed.
mods = []indexEntry{
{
modPath = strings.TrimSuffix(modPath, "/...") // No /... needed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Unclear to me end to end how module strings are moving through the system, do you ever need to sanitize trailing comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you mean by trailing comments. This comes from a command line argument. This makes sure that if the user did include the /... at the end, we handle it transparently. A previous version of this command passed the argument through directly, and I made use of /..., so I didn't want to break that.

mods = append(mods, indexEntry{
Path: modPath,
Version: version,
},
})
}
}

Expand All @@ -105,21 +106,19 @@ func main() {
return
}
// Create a temp module so we can get the exact version asked for.
tempDir, err := ioutil.TempDir("", "godocfx-*")
workingDir, err := ioutil.TempDir("", "godocfx-*")
if err != nil {
log.Fatalf("ioutil.TempDir: %v", err)
}
runCmd(tempDir, "go", "mod", "init", "cloud.google.com/go/lets-build-some-docs")
// Use a fake module that doesn't start with cloud.google.com/go.
runCmd(workingDir, "go", "mod", "init", "cloud.google.com/lets-build-some-docs")
for _, m := range mods {
log.Printf("Processing %s@%s", m.Path, m.Version)

path := *outDir
// If we have more than one module, we need a more specific out path.
if len(mods) > 1 {
path = filepath.Join(path, fmt.Sprintf("%s@%s", m.Path, m.Version))
}
if err := process(m, tempDir, path, *print); err != nil {
log.Printf("Failed to process %v", m)
// Always output to specific directory.
path := filepath.Join(*outDir, fmt.Sprintf("%s@%s", m.Path, m.Version))
if err := process(m, workingDir, path, *print); err != nil {
log.Printf("Failed to process %v: %v", m, err)
}
log.Printf("Done with %s@%s", m.Path, m.Version)
}
Expand All @@ -138,22 +137,23 @@ func runCmd(dir, name string, args ...string) error {
return nil
}

func process(mod indexEntry, tempDir, outDir string, print bool) error {
func process(mod indexEntry, workingDir, outDir string, print bool) error {
// Be sure to get the module and run the module loader in the tempDir.
if err := runCmd(tempDir, "go", "mod", "tidy"); err != nil {
return err
if err := runCmd(workingDir, "go", "mod", "tidy"); err != nil {
return fmt.Errorf("go mod tidy error: %v", err)
}
if err := runCmd(tempDir, "go", "get", mod.Path+"@"+mod.Version); err != nil {
return err
if err := runCmd(workingDir, "go", "get", "-d", "-t", mod.Path+"/...@"+mod.Version); err != nil {
return fmt.Errorf("go get %s@%s: %v", mod.Path, mod.Version, err)
}

log.Println("Starting to parse")
optionalExtraFiles := []string{}
filter := []string{
"cloud.google.com/go/analytics",
"cloud.google.com/go/area120",
"cloud.google.com/go/gsuiteaddons",
}
r, err := parse(mod.Path+"/...", tempDir, optionalExtraFiles, filter)
r, err := parse(mod.Path+"/...", workingDir, optionalExtraFiles, filter)
if err != nil {
return fmt.Errorf("parse: %v", err)
}
Expand Down
3 changes: 1 addition & 2 deletions internal/godocfx/parse.go
View file
Open in desktop
Expand Up @@ -452,8 +452,6 @@ func (l *linker) linkify(s string) string {
return fmt.Sprintf("%s%s.%s", prefix, href(l.toURL(pkgPath, ""), pkg), href(l.toURL(pkgPath, name), name))
}

// TODO: link to the right baseURL, with the right module name and version
// pattern.
func (l *linker) toURL(pkg, name string) string {
if pkg == "" {
if anchor := l.idToAnchor[""][name]; anchor != "" {
Expand All @@ -467,6 +465,7 @@ func (l *linker) toURL(pkg, name string) string {
pkgRemainder = pkg[len(mod.Path)+1:] // +1 to skip slash.
}
// Note: we always link to latest. One day, we'll link to mod.Version.
// Also, other packages may have different paths.
baseURL := fmt.Sprintf("/go/docs/reference/%v/latest/%v", mod.Path, pkgRemainder)
if anchor := l.idToAnchor[pkg][name]; anchor != "" {
return fmt.Sprintf("%s#%s", baseURL, anchor)
Expand Down