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

[FEATURE REQUEST] Build only the selected modules for golang #1995

Closed
pddg opened this issue Aug 2, 2021 · 4 comments
Closed

[FEATURE REQUEST] Build only the selected modules for golang #1995

pddg opened this issue Aug 2, 2021 · 4 comments

Comments

@pddg
Copy link

pddg commented Aug 2, 2021

Recursive submodule build is supported at #1788 .
This is a very useful feature and I appreciate it. However, in certain use cases, this will cause errors.

For example, the repository https://github.com/pddg/actionlint contains the following two main packages (this repository is forked from https://github.com/rhysd/actionlint ).

  • github.com/pddg/actionlint/cmd/actionlint
    • CLI tool to lint GitHub Actions workflow files
  • github.com/pddg/actionlint/playground
    • To be used in a browser, it is assumed to be built with GOOS=js and GOARCH=wasm

I added .pre-commit-hooks.yaml as follows.

https://github.com/pddg/actionlint/blob/pddg/pre-commit-hooks/.pre-commit-hooks.yaml#L1-L9

I want to build only the former, but both builds run and the latter fails.

$ cat .pre-commit-config.yaml
repos:
  - repo: https://github.com/pddg/actionlint
    rev: 28ff88c79af16f54232fca9299645b9ac318a862
    hooks:
      - id: actionlint
$ pre-commit run
[INFO] Installing environment for https://github.com/pddg/actionlint.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
An unexpected error has occurred: CalledProcessError: command: ('/bin/bash', '/Users/pudding/.asdf/shims/go', 'get', './...')
return code: 1
expected return code: 0
stdout: (none)
stderr:
    go: downloading github.com/yuin/goldmark v1.4.0
    go: downloading github.com/kr/pretty v0.2.1
    go: downloading gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b
    go: downloading github.com/fatih/color v1.12.0
    go: downloading github.com/mattn/go-colorable v0.1.8
    go: downloading github.com/mattn/go-runewidth v0.0.13
    go: downloading github.com/robfig/cron v1.2.0
    go: downloading golang.org/x/sync v0.0.0-20210220032951-036812b2e83c
    go: downloading golang.org/x/sys v0.0.0-20210630005230-0f9fa26af87c
    go: downloading github.com/mattn/go-isatty v0.0.12
    go: downloading github.com/kr/text v0.1.0
    go: downloading github.com/rivo/uniseg v0.2.0
    github.com/rhysd/actionlint/playground imports
        syscall/js: no Go source files
    package github.com/rhysd/actionlint/playground
        imports syscall/js: build constraints exclude all Go files in /Users/pudding/.asdf/installs/golang/1.16.5/go/src/syscall/js
    
Check the log at /Users/pudding/.cache/pre-commit/pre-commit.log

This is an example of including modules that will fail to build, but there may be other cases where you want to select modules to build in order to reduce build time.

Could you please consider building only selected modules?
Or, is there any workaround?

@asottile
Copy link
Member

asottile commented Aug 2, 2021

sounds like their thing is broken, not ours

@antdking
Copy link

antdking commented Feb 11, 2023

@asottile can you re-open this issue, please. It's not an upstream bug, more a side-effect of how pre-commit tries to install modules.

Using go install ./... breaks for monorepo layouts when portions need different build methods. It's also inefficient when you only need a portion of the repo built.
In Actionlint's case, /playground builds a WASM module, and expects to be compiled targeting JS.

Ideally, the hook author needs a way to specify a build target (in this case, go install ./cmd/actionlint).

A current workaround is to define a local hook with additional_dependencies defined, however this loses 2 key benefits:

  • It's on the user to write + maintain the hook, instead of the tool's authors
  • no pre-commit autoupdate support

I fully agree with your previous comments about trying to guess the path. I won't bother suggesting ./cmd/some-name will produce an executable called some-name, as it is just a convention (and doesn't hold try if ./cmd/some-name/go.mod exists).

A quick win could be to install additional_dependencies first, then only run go install ./... if the executable isn't built yet.
This is simpler to implement + test, and has performance benefits, but is subjectively a worse DX due to overloading additional_dependencies.

This would allow authors to do this:

entry: actionlint
additional_dependencies: [./cmd/actionlint]

For the edge case where additional_dependencies is already used, happen to generate the executable, and still need other executables built, Hook Authors should add ./... themselves, or the paths they need.

Does this sound reasonable?

@asottile
Copy link
Member

I don't think your solution works -- we don't know what "the executable" is (yes you could say the first part of entry, but that's not part of install, nor is it necessarily the executable (it could be env for instance)

pre-commit repos are heavily convention driven -- if something is not buildable then I don't think there's anything that can be done

@antdking
Copy link

Ah. I'd lost sight of the flexibility entry offers.
This case has a workaround anyway.

Thanks for the quick reply!

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

No branches or pull requests

3 participants