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

Draft: Runtime Dependency Checker (RDC) #107

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

Conversation

b1ek
Copy link
Member

@b1ek b1ek commented May 25, 2024

turns out implementing this wasn't as complicated as i thought.

features added:

  • the checker itself
    • cargo tests
  • CLI argument to disable it: --disable-rdc
  • the shebang header, which was missing for some reason

possible caveats:

  • i have needed to use regex, so a regex crate is added as well. It may increase the compiler binary size, but shouldn't be much.
  • RDC overrides variables AMBER_RDC_RD and AMBER_RDC_CD
  • RDC is a hardcoded bash script
  • RDC adds about 500 bytes to the compiled code (depends on used commands). It can be disabled via the command line option, though

known issues:

  • RDC can fail for an imported file, which is included in the middle of the code.
    • Im not sure about this one, as i don't know how imports work. Please correct me on this one.
  • RDC checks even commands referenced in unsafe $...$ blocks. I think its an issue to discuss, whether to ignore unsafe commands or create a new keyword for disabling RDC on one specific command.
  • RDC can't be disabled for imported files. I will probably fix this one when i have the energy.

Closes #95

src/modules/rdc.rs Outdated Show resolved Hide resolved
@Ph0enixKM
Copy link
Collaborator

Ph0enixKM commented May 25, 2024

Really good job implementing this @b1ek! Here is some bug that I found.

When I ran this code that used a standard library function split it failed with this error:
Screenshot 2024-05-25 at 20 47 49

It seems that it uses a predefined variable to set the IFS (Internal Field Separator) for just this command.
Screenshot 2024-05-25 at 20 48 09

Can we detect if a command is setting some variable before running something else? If so then can we find the real command? If not then we could omit or emit some kind of warning perhaps that the RDC is disabled for this command.

The bad part is that the variable assignment could be like this: IRC=" ". I think we have to parse the bash commands to some extent. Also there is this edge case where user could do this:

let command = "echo"
${command} "Hello World"$

@Ph0enixKM
Copy link
Collaborator

@boushley @arapower can you take a look at this?

@b1ek
Copy link
Member Author

b1ek commented May 25, 2024

Can we detect if a command is setting some variable before running something else?

Yeah, looks pretty simple

image

@b1ek
Copy link
Member Author

b1ek commented May 26, 2024

Also there is this edge case where user could do this:

let command = "echo"
${command} "Hello World"$

im not sure that a thing like that can be caught compile time, without rethinking the type system. perhaps we could create a type that would only allow certain strings, such as in typescript:

let cmd: 'echo' | 'curl';

and then check for all those strings in RDC.

but all of that seems as too much work for too little gain. my opinion is to let the user know that if they are specifying a command dynamically, RDC won't catch it. a workaround is possible:

silent unsafe $some_cmd --help$ // this is a no-op, but RDC will add this to its list
let command = "some_cmd"
${some_cmd} --stuff$

@b1ek
Copy link
Member Author

b1ek commented May 26, 2024

a workaround is possible:

silent unsafe $some_cmd --help$ // this is a no-op, but RDC will add this to its list
let command = "some_cmd"
${some_cmd} --stuff$

actually, it would be pretty neat to add a compiler flag to tell RDC what other commands should it check for, without impacting the output file

@Ph0enixKM
Copy link
Collaborator

Yeah, looks pretty simple

image

This doesn't seem to match the following string though:

IFS="hello world" read --arg1 --arg2

@b1ek
Copy link
Member Author

b1ek commented May 26, 2024

This doesn't seem to match the following string though:

IFS="hello world" read --arg1 --arg2

you're right.

this one should match any valid bash syntax: ^ *\S+=((\$|)\(.*\)|("|').*("|')|(\S+\\ |)+\S+|) *( *\S+=((\$|)\(.*\)|("|').*("|')|(\S+\\ |)+\S+|)|) *.

i love writing regex.

done

if (( ${#AMBER_RDC_MD[@]} != 0 )); then
>&2 echo This program requires for these commands: \( $AMBER_RDC_MD \) to be present in \$PATH.
Copy link
Contributor

Choose a reason for hiding this comment

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

The "for" in here breaks the english grammar. "This program requires these commands:" would be more correct.

@@ -49,6 +49,9 @@ impl TranslateModule for CommandStatement {
let interps = self.interps.iter()
.map(|item| item.translate(meta))
.collect::<Vec<String>>();

scan_append_externs(self.strings.clone(), meta);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is probably not something we want to try and do. Trying to understand all of the dependencies that a script relies on is a big ask. There could be dependencies that are set based on environment variables, the script could change the $PATH adding new things that were previously not present.

I think that in this initial pass we should add support for the Amber runtime dependencies. If we really want to provide this affordance to Amber users then it seems like it would be best to add a library function they can call to explicitly call out a dependency they want to check for.

In that way the script checks for the pieces that it depends on and then it provides a way for script uathors to do something siimlar.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh yeah. That's actually a good point that people could add things later on in the script to the $PATH. For now let's just leave it as a precheck for making sure that Amber script can at least run.

@b1ek
Copy link
Member Author

b1ek commented May 31, 2024

i feel like this should be maintained as a separate project, so it would work with any bash file, with a possibility of integrating it into amber later

@Ph0enixKM
Copy link
Collaborator

Ph0enixKM commented May 31, 2024

We should continue this discussion on Github Discussions that I'll create this evening. This PR is great and I think we can leave the basic RDC for Amber itself.

I'd remove the REGEX parsing of commands to search for the dependencies or I'd move them to a separate project. But I think that we need to discuss this whole idea first.

@arapower
Copy link
Collaborator

arapower commented Jun 3, 2024

By the way, wouldn't it be better to merge the following commit as a separate PR?

@b1ek
Copy link
Member Author

b1ek commented Jun 3, 2024

By the way, wouldn't it be better to merge the following commit as a separate PR?

* [2ff9022](https://github.com/Ph0enixKM/Amber/commit/2ff902212321f912614663600e4ad8ae542e152c)

yeah, i agree with that. opening a new PR now

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.

[Feature] Runtime dependencies checker?
4 participants