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

Compile check tests in CI #575

Merged
merged 19 commits into from
Feb 9, 2023

Conversation

ASpoonPlaysGames
Copy link
Contributor

@ASpoonPlaysGames ASpoonPlaysGames commented Jan 17, 2023

Uses ASpoonPlaysGames/squirrel-re-compiler (which uses RoyalBlue1/StandaloneR2Squirrel) to gather together scripts and try to compile them.

This currently would break if more functions are added via launcher, as it won't know that they exist. We should have a json file or something somewhere where the action can gather a list of function signatures added by native, alongside their VMs to avoid this problem

EDIT: this is resolved now using .github/nativefuncs.json

Closes #570

@ASpoonPlaysGames
Copy link
Contributor Author

Should close #570 if merged

Co-authored-by: GeckoEidechse <40122905+GeckoEidechse@users.noreply.github.com>
@uniboi
Copy link
Contributor

uniboi commented Jan 18, 2023

We should have a json file or something somewhere where the action can gather a list of function signatures added by native, alongside their VMs to avoid this problem

just make an extra script file that registers new launcher functions as global in the base suirrel files, that way you don't need to tamper with the compiler

@ASpoonPlaysGames
Copy link
Contributor Author

just make an extra script file that registers new launcher functions as global in the base suirrel files, that way you don't need to tamper with the compiler

Yeah that could work, however:

  1. It would still require manual updating of the compile check repo, and since it is a github action repo, thats not ideal
  2. We mess with the compiler a lot already, its basically northstar but with only the modloading and the adding global functions rn (as well as a roughly 8k line file adding every single native const to the VMs)

@uniboi
Copy link
Contributor

uniboi commented Jan 20, 2023

wouldn't the compiler / json / mod to load added functions be updated anyways every time a new signature is added? Unless the standalone compiler is being built in the same workflow as the launcher

@GeckoEidechse
Copy link
Member

GeckoEidechse commented Jan 23, 2023

just make an extra script file that registers new launcher functions as global in the base squirrel files, that way you don't need to tamper with the compiler

Yeah that could work, however:

1. It would still require manual updating of the compile check repo, and since it is a github action repo, thats not ideal

2. We mess with the compiler a lot already, its basically northstar but with only the modloading and the adding global functions rn (as well as a roughly 8k line file adding every single native const to the VMs)

What if we pass launcher functions as arg in CI file to action? So that way if a new launcher function is added, on the corresponding mods PR, all the author has to do is to add the function name to the CI file.

So it would be something like

- name: Compile Scripts
  uses: ASpoonPlaysGames/squirrel-re-compiler@v1
  with:
    mods-directory: "${{ github.workspace }}/mods"
    launcher-functions: "NSMakeHTTPRequest,my_other_custom_function,etc" # <--- this part

@GeckoEidechse
Copy link
Member

Alternatively @RoyalBlue1 suggested adding a JSON file with all the custom functions in this NorthstarMods repo and just putting it in .github/workflows/ so that it doesn't get added to releases.

@GeckoEidechse
Copy link
Member

Or embedding the JSON file into the CI config.

@ASpoonPlaysGames
Copy link
Contributor Author

Alternatively @RoyalBlue1 suggested adding a JSON file with all the custom functions in this NorthstarMods repo and just putting it in .github/workflows/ so that it doesn't get added to releases.

Sure, but we would have to enforce that this gets updated properly alongside any launcher PRs. (perhaps we could do some code commenting in launcher or something that we scan for in a workflow, and then just enforce it in code reviews?)

@GeckoEidechse
Copy link
Member

Alternatively @RoyalBlue1 suggested adding a JSON file with all the custom functions in this NorthstarMods repo and just putting it in .github/workflows/ so that it doesn't get added to releases.

Sure, but we would have to enforce that this gets updated properly alongside any launcher PRs. (perhaps we could do some code commenting in launcher or something that we scan for in a workflow, and then just enforce it in code reviews?)

Not sure if it that's even necessary. If the launcher added function is not added to the CI file (or JSON file or whatever) the mods PR CI will just fail and prevent the PR from being merged. At which point we can tell the author what to do to fix it.
(Though we should still have docs for it :P`)

@uniboi
Copy link
Contributor

uniboi commented Jan 24, 2023

just make an extra script file that registers new launcher functions as global in the base squirrel files, that way you don't need to tamper with the compiler

Yeah that could work, however:

1. It would still require manual updating of the compile check repo, and since it is a github action repo, thats not ideal

2. We mess with the compiler a lot already, its basically northstar but with only the modloading and the adding global functions rn (as well as a roughly 8k line file adding every single native const to the VMs)

What if we pass launcher functions as arg in CI file to action? So that way if a new launcher function is added, on the corresponding mods PR, all the author has to do is to add the function name to the CI file.

So it would be something like

- name: Compile Scripts
  uses: ASpoonPlaysGames/squirrel-re-compiler@v1
  with:
    mods-directory: "${{ github.workspace }}/mods"
    launcher-functions: "NSMakeHTTPRequest,my_other_custom_function,etc" # <--- this part

What about function signatures? Just the names isn't enough

@ASpoonPlaysGames
Copy link
Contributor Author

What about function signatures? Just the names isn't enough

Yeah we would need function signatures, not just names.

IIRC, the cpp code in launcher (and therefore squirrelstandalone) likes three strings (function name, return type, parameters), used to create the functions, as well as VM(s) to put the function in.

Example of func registration on cpp side: (empty string is the help text, not rly needed for compiling)

g_pSquirrel<ScriptContext::SERVER>->AddFuncRegistration(
        "void",
        "NSBroadcastMessage",
        "int fromPlayerIndex, int toPlayerIndex, string text, bool isTeam, bool isDead, int messageType",
        "",
        SQ_BroadcastMessage);
// void function NSBroadcastMessage( int fromPlayerIndex, int toPlayerIndex, string text, bool isTeam, bool isDead, int messageType )
SQRESULT SQ_BroadcastMessage(HSquirrelVM* sqvm)
{
    return SQRESULT_NULL;
}

So as an example you could have some json like this to represent a function which would then be added (as a stub, it'll never actually get ran) for compiling

{
    "name": "DoThing",
    "parameters": "string thing, int otherThing",
    "returnType": "array<bool>",
    "contexts": [ "UI", "CLIENT", "SERVER" ]
}

This would then be used to create a function like this: (obviously using variables not string literals)

g_pSquirrel<ScriptContext::SERVER>->AddFuncRegistration(
        "array<bool>",
        "DoThing",
        "string thing, int otherThing",
        "",
        SQ_StubFunc);
SQRESULT SQ_StubFunc(HSquirrelVM* sqvm)
{
    return SQRESULT_NULL;
}

@RoyalBlue1
Copy link
Contributor

So as an example you could have some json like this to represent a function which would then be added (as a stub, it'll never actually get ran) for compiling

for my new version I'm using this format, so I already have a parser for that
image

it also supports constants with the intConstants key

@ASpoonPlaysGames
Copy link
Contributor Author

So as an example you could have some json like this to represent a function which would then be added (as a stub, it'll never actually get ran) for compiling

for my new version I'm using this format, so I already have a parser for that image

it also supports constants with the intConstants key

Why are the returnType fields integers? An enum member or something?
Also do we ever have a different value for a3arg? I'm not a fan of including unknown variables in the json, could it be inferred from elsewhere?

@RoyalBlue1
Copy link
Contributor

there is returnType for the enum and returnTypeString for the string we use. the enum gets set in ns by a string compare but for native functions the string does not always exist so i needed to do it like this but if we only do ns functions it can be removed

in northstar functions arg3 is always 1 but i needed to set it for when i'm doing the other native functions so that could be removed as well

@ASpoonPlaysGames
Copy link
Contributor Author

there is returnType for the enum and returnTypeString for the string we use. the enum gets set in ns by a string compare but for native functions the string does not always exist so i needed to do it like this but if we only do ns functions it can be removed

in northstar functions arg3 is always 1 but i needed to set it for when i'm doing the other native functions so that could be removed as well

I think the best way to go about this would be to have 2 separate files, one for native functions that goes into base or something (that would never change because respawn aren't exactly updating the game) and one which you can specify the path for with a command line arg

The one which you specify the path for could use a simpler setup, only requiring the args needed for this (hoping to make keeping it updated easier with less confusion about the args)

g_pSquirrel<ScriptContext::SERVER>->AddFuncRegistration(
        "void",
        "NSBroadcastMessage",
        "int fromPlayerIndex, int toPlayerIndex, string text, bool isTeam, bool isDead, int messageType",
        "",
        SQ_BroadcastMessage);

whereas the one in base would use the full json format, since it won't need to be maintained much (if at all)

@RoyalBlue1
Copy link
Contributor

RoyalBlue1 commented Jan 24, 2023

we could also just have default values the same as ns so it behaves like ns but if you specify something it uses that

@ASpoonPlaysGames
Copy link
Contributor Author

we could also just have default values the same as values so it behaves like ns but if you specify something it uses that

Yeah that's what i meant, but i worded it badly. Splitting it into two files (one for native, one for NS) I think is the best move in that regard though

@ASpoonPlaysGames
Copy link
Contributor Author

o no

@ASpoonPlaysGames
Copy link
Contributor Author

should be working now

@GeckoEidechse GeckoEidechse changed the title Compile check tests Compile check tests in CI Feb 8, 2023
Copy link
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

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

Left 2 comments. Otherwise looks fine to me. We probably wanna add some docs somewhere describing what nativefuncs.json does. Not sure what the best approach is. README.md in .github folder?

Northstar.Client/mod/scripts/vscripts/ui/menu_lobby.nut Outdated Show resolved Hide resolved
.github/workflows/compile-check.yml Show resolved Hide resolved
Copy link
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

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

Looks good from my end ^^

@RoyalBlue1 RoyalBlue1 merged commit 12cf8b9 into R2Northstar:main Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Use standalone Squirrel for CI
4 participants