-
Notifications
You must be signed in to change notification settings - Fork 3
Prefer start module option to heuristics #45
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, but can we add a test for specifying a start_module that uses main/1 and assert that the packed avm has the correct module tagged as the start module?
Useful for defining a start module that doesn't have start/0 (but main/1). Signed-off-by: Paul Guyot <[email protected]>
1ad714c to
bb6c95c
Compare
UncleGrumpy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the test. I feel much better now about future changes not breaking anything. I did catch one other small detail I overlooked during my first review.
| case lists:member({start, 0}, Exports) of | ||
| true -> | ||
| if | ||
| StartModule =:= Module -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have thought of this sooner, but should we add a check here that init/1 is exported to make sure it's a valid start module before we set the flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a little bit confused about init/1 being exported since we don't have any init/1 function exported indeed (except maybe in gen_server or supervisor callback modules).
The point of this PR is that if user specifies the start module,
we should not use any heuristics and trust them. User in this case would know they have the proper init module in the libs partition.
We should get rid of the libs partition and have proper boot scripts, but this is not this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, I meant main/1. That was what I was wondering, do we blindly accept any start module (even if there is no currently supported mechanism for actually starting it), or should we make sure it’s valid and can be started by AtomVM?
I have no strong feeling, but it does seem preferable to at least warn the user (if not abort packing with an error) that the start module can’t be started by the VM, rather than wait until runtime and let the user sort out the runtime error.
Useful for defining a start module that doesn't have start/0 (but main/1).