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

Fix build for >1.9 #82

Closed
wants to merge 20 commits into from
Closed

Fix build for >1.9 #82

wants to merge 20 commits into from

Conversation

oscar-izval
Copy link
Contributor

@oscar-izval oscar-izval commented Jul 5, 2024

TODO:

  • wait for 1.22.4 to be in nixpkgs-unstable, then update flake.lock and try again

@sestrella
Copy link
Member

@oscar-izval I tried updating the "nixpkgs-unstable" input in the "flake.lock" file now that Terraform 1.9.2 is available on "nixpkgs". While this resolves the build issue, it consumes a significant amount of time and CI resources; I believe a workaround would be to introduce a new version of "nixpkgs-unstable" for 1.9. What are your thoughts?

@oscar-izval
Copy link
Contributor Author

@oscar-izval I tried updating the "nixpkgs-unstable" input in the "flake.lock" file now that Terraform 1.9.2 is available on "nixpkgs". While this resolves the build issue, it consumes a significant amount of time and CI resources; I believe a workaround would be to introduce a new version of "nixpkgs-unstable" for 1.9. What are your thoughts?

Yeah... not ideal, but not sure how sustainable the idea of adding new instances of nixpkgs is. The way I understand this, we'd need to add a new instance every time Terraform needs a newer version of Go, which is the reason we had to update the nixpkgs-unstable input here. In that case, I'd create a separate instance for each minor version of Go to "bundle" some versions together. While we are at it, I don't remember why we use nixpkgs for <=1.6, could we use unstable for everything?

On the other hand, CI time and resources are essentially free at this point, except for some babysitting if the build fails by being too big (I think that's why it failed last night). Maybe the most cost-effective solution here is to bite the bullet and rebuild everything.

@sestrella
Copy link
Member

@oscar-izval regarding:

In that case, I'd create a separate instance for each minor version of Go to "bundle" some versions together

Could you elaborate on this? Do you mean having a separate "nixpkgs" instance for each Go minor version needed to build Terraform?

While we are at it, I don't remember why we use nixpkgs for <=1.6, could we use unstable for everything?

It is primarily due to a license change on Terraform, which is declared as part of the derivation metadata.

https://github.com/NixOS/nixpkgs/blob/nixpkgs-unstable/pkgs/applications/networking/cluster/terraform/default.nix#L56

@oscar-izval
Copy link
Contributor Author

Could you elaborate on this? Do you mean having a separate "nixpkgs" instance for each Go minor version needed to build Terraform?

Yes, but now that I'm thinking about it again, we'll need to go in and add a new instance every time it's needed, which is not that different from babysitting the build and retriggering it a few times whenever we need a full recompile. From what I can see in the error logs, the issue is

> compile: writing output: write $WORK/b1210/_pkg_.a: no space left on device

We may want to look into it from that angle too. Link

@sestrella
Copy link
Member

Yes, but now that I'm thinking about it again, we'll need to go in and add a new instance every time it's needed, which is not that different from babysitting the build and retriggering it a few times whenever we need a full recompile.

I guess the main difference is that when we need to upgrade the Go version, it only affects new Terraform versions, while the previous ones remain unchanged and can be pulled from the binary cache. But let us talk more about this internally.

@sestrella
Copy link
Member

@oscar-izval While you started this PR, I believe I made some changes on top; in that sense, I would avoid the Obama medal approach here and instead open a PR where you could leave comments and give it your final approval. What are your thoughts?

@oscar-izval
Copy link
Contributor Author

Absolutely @sestrella, this work is all yours at this point. I'll make sure to take a look at your new PR :)

@sestrella
Copy link
Member

It is not about ownership; rather, it is about following up on the proper review process, in which someone else takes a closer look at the proposed changes. Thank you! I would then create a new PR.

@sestrella sestrella closed this Aug 6, 2024
@sestrella sestrella reopened this Aug 6, 2024
@sestrella sestrella closed this Aug 6, 2024
@sestrella sestrella deleted the fix-update branch August 7, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants