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

feat: override go builder to use overriden go #20

Merged
merged 3 commits into from
Sep 9, 2024
Merged

Conversation

dbarrosop
Copy link
Member

@dbarrosop dbarrosop commented Sep 9, 2024

PR Type

Enhancement


Description

  • Replaced custom buildGoNhostModule with standard buildGoModule in lib/go/go.nix
  • Renamed buildGoNhostModule to buildGoModule in overlays/go.nix for consistency
  • Updated golangci-lint configuration to use the new buildGoModule function
  • These changes streamline the Go build process and ensure consistent use of the overridden Go version

Changes walkthrough 📝

Relevant files
Configuration changes
go.nix
Update Go build function                                                                 

lib/go/go.nix

  • Changed pkgs.buildGoNhostModule to pkgs.buildGoModule
+1/-1     
Enhancement
go.nix
Refactor Go-related overlays                                                         

overlays/go.nix

  • Renamed buildGoNhostModule to buildGoModule
  • Updated golangci-lint to use prev.final.buildGoModule instead of a
    custom override
  • +2/-2     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @github-actions github-actions bot added the enhancement New feature or request label Sep 9, 2024
    Copy link
    Contributor

    github-actions bot commented Sep 9, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Potential Inconsistency
    The buildGoModule function is redefined with a different implementation compared to the one in lib/go/go.nix. This might lead to inconsistent behavior.

    Possible Oversight
    The golangci-lint override now uses prev.final.buildGoModule instead of prev.buildGoModule.override. This change might have unintended consequences and should be verified.

    Copy link
    Contributor

    github-actions bot commented Sep 9, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Include all relevant parameters in the inherit statement to ensure proper configuration

    Consider adding cgoEnabled to the list of inherited attributes in the buildGoModule
    call. This ensures that the cgoEnabled parameter is properly passed to the
    underlying build function.

    lib/go/go.nix [95-96]

     }: (pkgs.buildGoModule {
    -  inherit src version ldflags buildInputs nativeBuildInputs;
    +  inherit src version ldflags buildInputs nativeBuildInputs cgoEnabled;
     
    Suggestion importance[1-10]: 9

    Why: Including 'cgoEnabled' in the inherit statement is important for ensuring that all relevant parameters are properly passed to the build function, potentially preventing subtle configuration issues.

    9
    Best practice
    Use the final attribute instead of prev when overriding attributes to preserve modifications made in the current overlay

    Consider using the final attribute instead of prev when overriding buildGoModule.
    This ensures that any other modifications made to buildGoModule in the current
    overlay are preserved.

    overlays/go.nix [13]

    -buildGoModule = prev.buildGoModule.override { go = go; };
    +buildGoModule = final.buildGoModule.override { go = go; };
     
    Suggestion importance[1-10]: 8

    Why: Using 'final' instead of 'prev' is a best practice in Nix overlays, as it ensures that any modifications made to buildGoModule in the current overlay are preserved.

    8
    Maintainability
    Add a comment to explain the purpose of overriding buildGoModule

    Consider adding a comment explaining why buildGoModule is being overridden with a
    custom go version. This will help other developers understand the purpose of this
    modification.

    overlays/go.nix [13]

    +# Override buildGoModule to use our custom Go version
     buildGoModule = prev.buildGoModule.override { go = go; };
     
    Suggestion importance[1-10]: 6

    Why: Adding a comment improves code maintainability by explaining the reason for the override, which is helpful for other developers but not critical for functionality.

    6

    @dbarrosop dbarrosop merged commit 7cafee7 into main Sep 9, 2024
    2 checks passed
    @dbarrosop dbarrosop deleted the override-go-build branch September 9, 2024 06:11
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant