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

Update make based instructions #260

Merged
merged 13 commits into from
Jul 3, 2024
Merged

Conversation

emlautarom1
Copy link
Contributor

@emlautarom1 emlautarom1 commented Jun 21, 2024

Current Behavior

make start-operator errors out immediately with the following error:

{"err":"open tests/keys/bls/1.bls.key.json: no such file or directory"}

More details on #257

New Behavior

This PR updates make based instructions to match the current Docker setup.

Breaking Changes

None beside getting something to work that was previously broken.

Observations

  • Added a new near-da-rpc-sys make command that builds the native libraries required by the relayer.
  • Adjusted the CGO linker flags so the relayer can pick the required native libraries during build time.
  • Updated README with instructions on how to build the native libraries.
  • Updated config-files/operator.anvil.yaml to match the latest file structure regarding keys.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@taco-paco taco-paco left a comment

Choose a reason for hiding this comment

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

I would propose to refrain from getting rollup-data-availability as submodule.

We shall list rollup-data-availability artifacts as prerequisites. Maybe with explanation on how to compile it.

The compilation of relayer is platform-dependant and rollup-data-availability handles that here and here.

User shall invoke make command here and then move it to /usr/local/lib. Go will pick it up itself after that

@firatsertgoz
Copy link

May I suggest another angle,

How about instead of using da-rpc-sys you straight up use the http-api. Yes you would need to run a sidecar with the relayer, but the seperation would be very clean.

@taco-paco
Copy link
Contributor

May I suggest another angle,

How about instead of using da-rpc-sys you straight up use the http-api. Yes you would need to run a sidecar with the relayer, but the seperation would be very clean.

That is a great idea. @emlautarom1 do you want to play with it? This would require small Rust service on top of rollup-data-availability.

@firatsertgoz
Copy link

May I suggest another angle,
How about instead of using da-rpc-sys you straight up use the http-api. Yes you would need to run a sidecar with the relayer, but the seperation would be very clean.

That is a great idea. @emlautarom1 do you want to play with it? This would require small Rust service on top of rollup-data-availability.

We already have a service for this in the repo! https://github.com/Nuffle-Labs/data-availability/blob/main/bin/http-api/src/main.rs
Just need to run the binary at the same time with the relayer, POST and GET calls through relayer in Go. No need to do anything in Rust :)

@emlautarom1
Copy link
Contributor Author

I would propose to refrain from getting rollup-data-availability as submodule.

We shall list rollup-data-availability artifacts as prerequisites. Maybe with explanation on how to compile it.
User shall invoke make command here and then move it to /usr/local/lib. Go will pick it up itself after that

Initially I worked on a different approach that relied on cloning the rollup-data-availability repo, run the appropriate make command, copied the artifacts and then deleted the repo. This removes the need for the submodule and ensures that we're building for the correct platform, without introducing additional friction.

I'm against installing the libraries globally (ex. /usr/local/lib), so we would like to "install" them locally (this PR uses relayer/libs). The issue is then how do we ensure that the Go compiler includes that path during the linking phase without polluting the global defaults.

I think defining the make command to something like CGO_LDFLAGS="-L ./relayer/libs" go run ... should be enough.

The compilation of relayer is platform-dependant and rollup-data-availability handles that here and here.

My understanding is that our relayer (relayer/) is not platform dependent but rather it has a dependency on native libraries. What we need to do is to point Go to the correct directory while compiling the relayer to properly pick up these libraries.

@emlautarom1
Copy link
Contributor Author

Regarding the alternative proposed by @firatsertgoz, I think it would be nice to explore it later. For now I would like to fix the current make based instructions without adding additional dependencies, in particular considering that the docker-compose workflow is already working as intended (and it uses da-rpc-sys under the hood).

- Scope `CGO_LDFLAGS` to single command
@Hyodar
Copy link
Contributor

Hyodar commented Jul 1, 2024

About the Go sidecar conversation - I agree with both. We should move to the Go sidecar but IMO this PR should keep its scope.

Reason for that is already in #165. We can move ahead with it, but I'd still prefer if it kept a compatibility layer in this case. AFAIK we don't have other breaking updates piled up and coordinating those should be very costly - there are nodes from all over the world and some are more responsive than others.

@taco-paco
Copy link
Contributor

taco-paco commented Jul 1, 2024

@Hyodar thanks for implicitly reminding about the PR)
With regards to sidecar we can just use the current commit of near-da, thus it shall be only relayer update. After we would probably be able to discard #165 and just update the sidecar along with operator.

@emlautarom1

I'm against installing the libraries globally (ex. /usr/local/lib), so we would like to "install" them locally (this PR uses relayer/libs). The issue is then how do we ensure that the Go compiler includes that path during the linking phase without polluting the global defaults.

The thing is that CGO_LDFLAGS shall be customizable by user himself not by us. There're multiple C libraries that our code depends on implicitly handled by go itself we can't go with the way of trying to specify them for users. If user has custom locations with zlib, glibc, rollup-da he can use CGO_LDFLAGS himself.

Unfortunately there's no such thing as build.rs in GO

@Hyodar
Copy link
Contributor

Hyodar commented Jul 1, 2024

Okay, all conversation aside, I think the first question we should answer here is whether this tooling is important atm. It was used before and increasingly unused over time. Are we using these individual commands, or should we just deprecate this and use exclusively docker compose containers? On my end, I usually just use the containers directly, as it's actually closer to the real scenario.

@emlautarom1
Copy link
Contributor Author

Are we using these individual commands, or should we just deprecate this and use exclusively docker compose containers? On my end, I usually just use the containers directly, as it's actually closer to the real scenario.

I favor deprecating the make based instructions and only use docker as the unique workflow (less maintenance burden) but that's outside the scope of this PR. Note that we should still have make for development purposes (ex. run tests, build images, etc.)

@Hyodar
Copy link
Contributor

Hyodar commented Jul 2, 2024

Okay, after checking the whole conversation again, we can generally move forward with this. My overall take is:

  • Depending on a global library to be managed by the user is not nice as this is a quite specific application. We should try to keep things local as much as possible.
  • This is going to be deprecated (really) soon, I don't think we should spend more headspace with this

That said, I understand the trouble here, but overall I think defining the library as it is the best approach for now, though I'd highly prefer if we concatted (e.g. CGO_LDFLAGS += -L/path/to/your/library) instead of redefining the env variable. This way the user is not tied to our definition. The compilation instructions should also be updated in the docs.

Again, this should be deprecated really soon in favor of only container-based testing, so we don't have much to gain on this discussion.

@emlautarom1
Copy link
Contributor Author

That said, I understand the trouble here, but overall I think defining the library as it is the best approach for now, though I'd highly prefer if we concatted (e.g. CGO_LDFLAGS += -L/path/to/your/library) instead of redefining the env variable. This way the user is not tied to our definition

Done, now we concatenate existing CGO_LDFLAGS if any, allowing the user to configure this behavior if needed.

The compilation instructions should also be updated in the docs.

Already updated the main README.md with new instructions as described in the PR body.

Copy link
Contributor

@Hyodar Hyodar left a comment

Choose a reason for hiding this comment

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

LGTM! Please update the docs/ intro page as well.

@Hyodar
Copy link
Contributor

Hyodar commented Jul 3, 2024

Added the deprecation of the non-container testing environment instructions as #269.

@emlautarom1
Copy link
Contributor Author

Merging as discussed during today's daily

@emlautarom1 emlautarom1 merged commit 49a79e3 into main Jul 3, 2024
5 checks passed
@emlautarom1 emlautarom1 deleted the fix/make-based-instructions branch July 3, 2024 15:26
@emlautarom1 emlautarom1 linked an issue Jul 3, 2024 that may be closed by this pull request
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.

make start-operator not working correctly
5 participants