Nuitka / Nuitka-Action

Action to build with Nuitka on GitHub in your workflows
MIT License
111 stars 22 forks source link

Unable to provide multiple options with same key #38

Closed almightychang closed 5 months ago

almightychang commented 5 months ago

Intro

Trying to implement github action for following CLI command:

python3 -m nuitka main.py --include-data-dir=A=A --include-data-dir=B=B

As uniqueness of the keys is required in yaml file, I provide following option on Nuitka-action

uses: nuitka/Nuitka-Action@main
with:
    include-data-dir: A=A,B=B

However, it was translated into include A into A,B directory. It seems that the nuitka cannot parse multi-value on single key input.

About workflow option parsing in Nuitka

Nuitka workflow option parsing code has lack of support on multi-value.

    for key, value in json.loads(os.environ["NUITKA_WORKFLOW_INPUTS"]).items():
        if not value:
            continue

        if not filterByName(key):
            continue

        option_name = "--%s" % key

        if parser.isBooleanOption("--%s" % key):
            if value == "false":
                continue

            options_added.append(option_name)
        else:
            # Boolean disabled options from inactive plugins that default to off.
            if value == "false":
                continue

            options_added.append("%s=%s" % (option_name, value))

    sys.argv = (
        sys.argv[: github_option_index + (1 if phase == "early" else 0)]
        + options_added
        + sys.argv[github_option_index + 1 :]
    )

I think _considerGithubWorkflowOptions() should support multi-value parsing, as CLI supports multi-key. Is there any solution in this case? or am I right?

kayhayen commented 5 months ago

Hello,

yes, some options do that, some dont. You should be able to use a list value in the yaml though, and then it would work.

Yours, Kay

kayhayen commented 5 months ago

What I mean, is that it should be solved in the general parsing of Nuitka, not the GitHub Action related one. The value would have to be good on the command line first.

almightychang commented 5 months ago

I have same opinion about general parsing, but I got unexpected behavior with this:

uses: nuitka/Nuitka-Action@main
with:
    include-data-dir: A=A,B=B

This action supposed to work as --include-data-dir=A=A --include-data-dir=B=B.

root
   - A
   - B

However, resulting file structure was

root/
   - A,B/

This implies that general parsing did not splitted the options with comma (,).

kayhayen commented 5 months ago

No, it doesn't. I think that general parsing in Nuitka, is implemented for options that take module names, plugin names, etc. but never filename taking options. I guess, we would have to support escapes there, but I never saw any application take commas, or maybe I just forgot. But I do not really want to add \, support, or support people for who filenames with commas start to fail. It's probably bad enough that Nuitka treats = special in those options already.

For now the Nuitka-Action works as designed. The yaml parser doesn't turn comma containing strings into lists, and that's fine.

Once we generate the options, we could consider that list value options, should have the plural form, or maybe enforce the value to be a list value, as some kind of schema check. I totally can see how it's difficult to know how to get from one value to multiple, that is inconsistent.

For now, that parsing code uses no knowledge about those options. It would e.g. be prudent to make a single string value an error, if it contains a str value with a "," for a list item.

So I am leaving this open for until I find the time to generate the action description automatically from the Nuitka options declaration code.

almightychang commented 5 months ago

Alright, I understand the inconsistency from comma separated style. What I understood now is:

  1. Github Action does not allow input in list type. https://github.com/actions/toolkit/issues/184
  2. It is not a good idea to parse multiple value from single string, which also corresponds to Nuitka's general parser.

Can you provide any workarounds to deal with multiple value for a option from action? Or is it not available as of right now?

almightychang commented 5 months ago

I remember some options for list input (ex. --include-data-dirs) in old version of Nuitka-Action. Is it deprecated at all?

kayhayen commented 5 months ago

You are not giving list input. You are giving a str input in your yaml. A list input would be - a\n-b and the like.

Previously in shell and PowerShell script, there was a manual and incomplete conversion, that was fundamentally changed to be replaced with the code you quoted. It may have done that for some options, but I think it shouldn't have.

For me, the yaml should be 1:1 mapping of the command line options where it is about those.

almightychang commented 5 months ago

I totally agree that yaml format can cover the command line options, but the problem is Github Action's Syntax as it is not 1:1 mapping with yaml format. List input is not supported in Github Actions, although yaml supports it.

I tried this, but no success:

A list input would be - a\n-b and the like.

uses: nuitka/Nuitka-Action@main
with:
    include-data-dir: 
        - A=A
        - B=B

Also, capture from Github Web editor lints about it: image

almightychang commented 5 months ago

I found some workarounds for list input in github actions here: google-github-actions/get-secretmanager-secrets. It uses comma or new line to split the list. I think new line may be an alternative.

kayhayen commented 5 months ago

Can you try this, with develop version of Nuitka used:

uses: nuitka/Nuitka-Action@main
with:
    include-data-dir: |
        A=A
        B=B

For 2.0 I think this should be working now, although I have conflicting reports, the code however should properly split new lines now for list options.

kayhayen commented 5 months ago

The conflicting report was using main which doesn't have the necessary parser change yet that is so far only on develop.

almightychang commented 5 months ago

I will check with my team and report here. Thanks.

kayhayen commented 5 months ago

I am assumin this is solved.