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

Retrieve and build protobuf from remote URLs #158

Merged
merged 7 commits into from
Mar 13, 2024
Merged

Conversation

wilyle
Copy link
Contributor

@wilyle wilyle commented Mar 13, 2024

Currently, Freyja is using cargo git dependencies to reference the proto packages from other ESDV projects such as Ibeji. This has caused compatibility issues and needs to be updated. This PR makes a change to pull the content from a remote URL and build the proto files within the Freyja repo. Inspired by UProtocol: https://github.com/eclipse-uprotocol/up-rust/blob/main/build.rs

Also makes a minor update to the install instructions in the README, which required updating the markdown-ci workflow due to a recent regression in the tool that it uses (see tcort/markdown-link-check#304)

Fixes #157

Copy link
Contributor

@ashbeitz ashbeitz left a comment

Choose a reason for hiding this comment

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

A few comments. Please take a look.

use freyja_build_common::compile_remote_proto;

const CHARIOTT_SERVICE_DISCOVERY_INTERFACES_BASE_URI: &str =
"https://raw.githubusercontent.com/eclipse-chariott/chariott/main/service_discovery/proto";
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure how static "raw.githubusercontent.com" is. Do others rely on this domain name?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we rely on a specific version, rather than the latest?

Copy link
Contributor Author

@wilyle wilyle Mar 13, 2024

Choose a reason for hiding this comment

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

raw.githubusercontent.com is the domain for getting raw file contents. As I understand this is static and is the convention that github uses for accessing raw file contents. If you navigate to a file in github and click "Raw", you will go to this domain. This is the same as what UProtocol is doing, see the link in the PR description.

As for versioning, I'd prefer to stick with main for now to avoid having a "hidden dependency" in these files that needs to be maintained manually and separately from all other dependencies. Besides, the file paths have a versioning convention (everything used here has v1 in the file path), so I would expect that the files we're referencing should at least be backwards compatible if not unchanged even when using the main branch here. If this proves untrue or otherwise problematic we can revisit this usage. To compare to UProtocol, they are using a specific tag for their internal contracts and main for the external cloudevent definitions.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is what GitHub's Copilot said:

"Yes, raw.githubusercontent.com is a service provided by GitHub to serve raw files from GitHub repositories. It's used to access the raw content of files hosted on GitHub. However, keep in mind the content served by this service is directly linked to the content of the repository, so if a file or repository is altered or deleted, the raw content served will also change or become unavailable."

So it looks like it is a location that we can rely on long term.

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree with using the latest, but we can agree to disagree on this one.

Copy link
Contributor

Alex Recommends Report

Alex recommends the following language changes, but Alex is a regular expression based algorithm, so take them with a grain of salt.

✨ 🚀 ✨ Nothing to Report ✨ 🚀 ✨

ashbeitz
ashbeitz previously approved these changes Mar 13, 2024
Copy link
Contributor

@ashbeitz ashbeitz left a comment

Choose a reason for hiding this comment

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

Approved with one disagreement.

ashbeitz
ashbeitz previously approved these changes Mar 13, 2024
Copy link
Contributor

@ashbeitz ashbeitz left a comment

Choose a reason for hiding this comment

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

Approved with a disagreement on versioning.

Copy link
Contributor

@ashbeitz ashbeitz left a comment

Choose a reason for hiding this comment

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

Approved with disagreement on versioning.

@wilyle wilyle merged commit 7177fdc into main Mar 13, 2024
11 checks passed
@wilyle wilyle deleted the wilyle/proto-build branch March 13, 2024 22:35
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.

[enhancement] Retrieve and build protobuf from remote URLs
2 participants