Skip to content

chore: Replace acap-build crate with external rs4a-eap - #239

Draft
apljungquist wants to merge 2 commits into
AxisCommunications:mainfrom
apljungquist:delegate-acap-build
Draft

chore: Replace acap-build crate with external rs4a-eap#239
apljungquist wants to merge 2 commits into
AxisCommunications:mainfrom
apljungquist:delegate-acap-build

Conversation

@apljungquist

Copy link
Copy Markdown
Collaborator

Note

This is still work in progress, I still want to land a few more changes in rs4a and publish the crate.
That said, early feedback is welcome, in particular on issues such as:

  • whether the factoring out is at all acceptable
  • pushing environment variable parsing into the CLI; this is kind of a drive by change but it's a practice that has grown on me since it keeps library functions pure and makes it easy for users of a CLI to see all inputs that affect the behavior of the program

Remove the in-tree acap-build crate and depend on the rs4a-eap crate from apljungquist/rs4acap instead. This has benefits in two categories:

  • Clarifies the purpose of this repo as facilitating the use of Rust in ACAP app implementations
  • rs4a-eap has several implemented and planned improvements, notably the option to not depend on external programs allowing it to be fully installed with cargo, and more robust testing.

We drop all use of the reference implementation, which rs4a does not support and instead rely on the testing done by the library maintainer (me). The approach was flawed anyway since bugs could creep in when staging files in a temporary directory.

Footers

Related-to: #205 (abandoned attempt at not shelling out to cp, a problem that does not affect rs4a-eap)

Remove the in-tree `acap-build` crate and depend on the `rs4a-eap` crate from apljungquist/rs4acap instead.
This has benefits in two categories:
- Clarifies the purpose of this repo as facilitating the use of Rust in ACAP app implementations
- `rs4a-eap` has several implemented and planned improvements, notably the option to not depend on external programs allowing it to be fully installed with cargo, and more robust testing.

We drop all use of the reference implementation, which rs4a does not support and instead rely on the testing done by the library maintainer (me). The approach was flawed anyway since bugs could creep in when staging files in a temporary directory.
.args(args)
.artifact_dir(get_cargo_metadata(None)?.target_directory.join("acap"))
.execute()?;
.execute(cli.source_date_epoch.unwrap_or_default())?;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This doesn't actually match the docstring:

Defaults to the current time.

I need to fix that.

I may move the interpolation into execute or some other shared location.

@apljungquist apljungquist changed the title Replace acap-build crate with rs4a-eap from git chore: Replace acap-build crate with external rs4a-eap Jul 9, 2026
@socket-security

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Updatedcargo/​clap@​4.5.13 ⏵ 4.5.609910093100100

View full report

@guoxe guoxe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not against this change. Especially as more parts of the ACAP SDK gets added it would be nice to focus on only the wrappers and example applications and having any additional tooling live elsewhere.
This way, the focus of this repo is clearer and there is really nothing that requires one to use the extra tooling either, using cargo and existing ACAP tools works just as well as the experimental ones.

Completions(CompletionsCommand),
}

fn parse_mtime(s: &str) -> anyhow::Result<Mtime> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Might be worth DRYing this since to me it does not seem like we would want the implementations to diverge.

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