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

refactor(let): Cleaned up the exec ecu code a lot #184

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

DanielVoogsgerd
Copy link
Member

I needed to clean this up a bit to make it easier to follow. I could refactor more, but I have to get back to other stuff now.

brane-let/src/exec_ecu.rs Dismissed Show dismissed Hide dismissed
brane-let/src/exec_ecu.rs Fixed Show fixed Hide fixed
brane-let/src/exec_ecu.rs Fixed Show fixed Hide fixed
brane-let/src/exec_ecu.rs Fixed Show fixed Hide fixed
/// * `function`: The function name to execute in the package.
/// * `arguments`: The arguments, as a map of argument name / value pairs.
/// * `working_dir`: The wokring directory for this package.
/// Arguments:
Copy link
Member

Choose a reason for hiding this comment

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

Nowadays I'm using # Arguments everywhere, which plays nice with rustdoc. But not work blocking this PR on IMO

Copy link
Member Author

Choose a reason for hiding this comment

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

I will fix it, I really wish Rust would take a stance on what is prefered, but while there was some sort of convention in the past, that convention has deliberately been removed from the documentation.

Copy link
Member

Choose a reason for hiding this comment

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

Nice. That sounds convenient!

brane-let/src/exec_ecu.rs Outdated Show resolved Hide resolved
@Lut99
Copy link
Member

Lut99 commented Nov 4, 2024

I've added fixes for 3/4 of yours FIXMEs ~ feel free to provide thoughts :)

I'm unable to test if it works, though. I don't have access to a VM that compiles branelet with the appropriate GLIBC dependency, and the make.py is broken in this branch so I can't run the in-container compilation. I don't suppose you have a setup available to make it work?

Longer-term fix: bring back in-container compilation. Workaround: #191 and update the Dockerfile to an Ubuntu version that is more reasonable to compile to.

@DanielVoogsgerd
Copy link
Member Author

I think we can compile it using the new Makefile, however I am more limited by my broken Brane configuration which I am still trying to figure out. I will take a look once that is fixed (more important than this PR anyway). Thanks for the fixes, though!

DanielVoogsgerd and others added 2 commits November 14, 2024 14:13
- Remove old commented code
- Use more rusty language features (more consise)
Test pending, need to try out on a VM to get `branelet` container compatability lol
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