Downstream AtomVM, simplify further downstreaming #151
Conversation
mat-hek
force-pushed
the
mf/popcorn-nifs
branch
3 times, most recently
from
0e8c250 to
48b9dfa
Compare
.github/workflows/build_test.yml
Outdated
| push: | ||
| branches: ["**"] |
There was a problem hiding this comment.
Do we need a build on push? Maybe on pull request would suffice?
There was a problem hiding this comment.
Is it problematic assuming we enable the concurrency? If not, I'd leave it, it's nice to have a status next to each commit, no matter if a PR is open
There was a problem hiding this comment.
If after making it public we don't have CI minutes limit, then it's fine
.github/workflows/build_test.yml
Outdated
| on: | ||
| push: | ||
| branches: ["**"] | ||
| pull_request: | ||
| branches: ["**"] |
There was a problem hiding this comment.
If we filter nothing, we might as well skip branches
| on: | |
| push: | |
| branches: ["**"] | |
| pull_request: | |
| branches: ["**"] | |
| on: [push, pull_request] |
or even
| on: | |
| push: | |
| branches: ["**"] | |
| pull_request: | |
| branches: ["**"] | |
| on: pull_request |
| branches: ["**"] | ||
| pull_request: | ||
| branches: ["**"] | ||
|
|
There was a problem hiding this comment.
If using both push and pull_request, I'd add concurrency like in popcorn and original workflows
| concurrency: | |
| group: ${{ github.workflow }}-${{ github.ref != 'refs/heads/main' && github.ref || github.run_id }} | |
| cancel-in-progress: true |
There was a problem hiding this comment.
We could actually use that to have consistency in formatting
There was a problem hiding this comment.
Tried enabling it, but one of the checks fails to run: https://github.com/software-mansion-labs/FissionVM/actions/runs/17796124092/job/50584059031 I'm not really into debugging it :P
There was a problem hiding this comment.
Lol, its simply a matter of repo name - on line 69
it should be
cd ../FissionVM
src/libAtomVM/nifs.c
Outdated
| fprintf(stderr, "nif not found\n"); | ||
| AVM_ABORT(); | ||
|
|
||
| RAISE_ERROR(UNDEF_ATOM); |
There was a problem hiding this comment.
This code is unreachable, can you delete it?
There was a problem hiding this comment.
Could we further break down this file and have nifs from each erlang/elixir module in separate file? IMO this file should only contain structs for NIFs and popcorn_nifs_get_nif.
This can be in a separate PR though
There was a problem hiding this comment.
Not sure, since most of these NIFs will be removed once we upstream them, and I'd generally expect to have only a few NIFs here
src/libAtomVM/popcorn/popcorn_md5.c
Outdated
| free(input_buffer); | ||
|
|
||
| memcpy(result, ctx.digest, 16); | ||
| } No newline at end of file |
| static const struct Nif ets_new_nif = { | ||
| .base.type = NIFFunctionType, | ||
| .nif_ptr = nif_ets_new | ||
| }; |
There was a problem hiding this comment.
This is very repetitive, could we handle it with a macro?
There was a problem hiding this comment.
I think it should rather be similar to nifs.c, so we can move NIFs around (fairly) easily and to keep it simple
| @@ -0,0 +1,1114 @@ | |||
| // See popcorn_nifs.gperf for status of each NIF | |||
There was a problem hiding this comment.
IMO this status should be here assuming only structs will be here
mat-hek
force-pushed
the
mf/popcorn-nifs
branch
3 times, most recently
from
1a0aa64 to
eca8e05
Compare
mat-hek
force-pushed
the
mf/popcorn-nifs
branch
from
eca8e05 to
c5d13f8
Compare
mat-hek
force-pushed
the
mf/popcorn-nifs
branch
from
e29be0d to
40ba8e0
Compare
closes software-mansion/popcorn#290
Requires changes in Popcorn: software-mansion/popcorn#301