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

Share Go module between node-installer and contrast #775

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

msanft
Copy link
Contributor

@msanft msanft commented Jul 31, 2024

Since the Contrast main package (i.e. cli, etc.) will need to share a significant portion of code to enable runtime-handler calculation for both, we transition to using the top-level module for the node-installer to enable imports of the internal packages of the Contrast main module.

Alternative options considered included code-sharing, which would introduce many duplications accross node-installer and main module, and would have required a duplicated injection of the reference values, and thus was considered infeasible. Another solution tried consisted of moving the platforms package to a top-level exported package in the main module, but since that node-installer is a child of the main module too, a replace tag does not work in this case, as the node-installer in github.com/edgelesssys/contrast/node-installer is unable to use a replacement for github.com/edgelesssys/contrast.

@msanft msanft added the no changelog PRs not listed in the release notes label Jul 31, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite get why this is needed - it looks like we've been using github.com/edgelesssys/contrast/node-installer/platforms in the CLI before, so why can't we do this anymore with the runtime handler calculation?

Copy link
Contributor Author

@msanft msanft Jul 31, 2024

Choose a reason for hiding this comment

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

Since the runtime handler calculation depends on the embedded reference values (as they include the hashes of the runtime components).

This would require us to inject the reference values into both the node-installer and the main module, also requiring code duplication for the calculation.

We could also move the reference values (and the embedding) into the node-installer entirely, but that felt out-of-place, since the main usage of these happens in the manifest. But if you see an advantage in doing so, I'm happy to adjust.

Copy link
Contributor

Choose a reason for hiding this comment

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

But then we still need to duplicate the asset injection, don't we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I could image now, we'd only need to inject into the node-installer then?

Copy link
Contributor

Choose a reason for hiding this comment

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

We still need to pass the linker flags to both binaries, but we don't need to pass two linker flags for the same version in different modules. We could also declare the linker flags in a single place and reuse that for all relevant binaries.

@msanft msanft requested a review from burgerdev July 31, 2024 08:07
Copy link
Member

@katexochen katexochen left a comment

Choose a reason for hiding this comment

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

Why aren't we just building the node-installer as output of contrast then?

packages/by-name/contrast-node-installer/package.nix Outdated Show resolved Hide resolved
packages/by-name/contrast-node-installer/package.nix Outdated Show resolved Hide resolved
@msanft msanft requested a review from katexochen July 31, 2024 09:48
@msanft msanft force-pushed the msanft/share-package branch 4 times, most recently from 43cca7e to e7978c1 Compare July 31, 2024 10:04
Since the Contrast main package (i.e. cli, etc.) will need to share a significant portion of code to enable runtime-handler calculation for both, we transition to using the top-level module for the node-installer to enable imports of the internal packages of the Contrast main module.

Alternative options considered included code-sharing, which would introduce many duplications accross node-installer and main module, and would have required a duplicated injection of the reference values, and thus was considered infeasible. Another solution tried consisted of moving the platforms package to a top-level exported package in the main module, but since that node-installer is a child of the main module too, a replace tag does not work in this case, as the node-installer in `github.com/edgelesssys/contrast/node-installer` is unable to use a replacement for `github.com/edgelesssys/contrast`.
@msanft msanft merged commit 1c73e06 into main Jul 31, 2024
11 checks passed
@msanft msanft deleted the msanft/share-package branch July 31, 2024 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog PRs not listed in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants