Repository navigation
NaCl process launching cleanup - #2037
Conversation
On arm64 Linux there is a half-attempt to fall back to box64 if the armhf loader can't start. This was merged by mistake; it depends on the double bootstrap loader which does not exist in the final version of DaemonEngine#2018. The process exit code detection junk which depended on that must be removed. I will add a working version of armhf->box64 fallback later.
Handle all setup of the NaCl bootstrap loader in a single block of code, instead of spraying little pieces and ifdefs all over the place. This was supposed to be merged as part of the "Don't use nacl_helper_bootstrap with x86-64 loader" commit in DaemonEngine#2001.
|
Hmmm, it looks like I forgot a commit when merging the rebase. |
|
I read the code in If the armhf compatibility detection is failing, then only this part has to be edited. |
|
This simplifies and unscatters the bootstrap code while keeping the loader command line construction properly deduplicated: |
| std::string box64Path = ResolveBox64Path(naclPath); | ||
| Log::Notice("Using Box64 emulator: %s", box64Path); | ||
| args.push_back(box64Path.c_str()); | ||
| inheritEnvironment = true; |
There was a problem hiding this comment.
This will use box64 unconditionally even if another architecture is supported and preferred, and will even attempt to run the armhf nexe with box64.
There was a problem hiding this comment.
Sorry for the approval noise, GitHub switched me to some new interface that is messed up and I don't even know how to dismiss the approval neither how to return to the good old interface.
There was a problem hiding this comment.
This will use box64 unconditionally even if another architecture is supported and preferred, and will even attempt to run the armhf nexe with box64.
There is currently no possibility in CMake to have more than one supported NaCl runtime for any Linux platform. armhf_usage and box64_usage do not intersect. If box64 is enabled, it is the only way.
There was a problem hiding this comment.
Everything is fully implemented since maybe 6 months, and what is not merged yet is just waiting in line to be.
There is no reason to re-introduce limitations that were already removed, and to reintroduce incompatibilities that were already solved.
Not claiming that it's broken in this way. The code is just an unreadable mess. |
|
There are good reasons why I spent several weeks six months ago untangling all this code and carefully removing all duplicate instances of command construction, I certainly didn’t do that just to reintroduce new duplication. |
|
If broken code can be pushed to master without review, it is only fair that it can be fixed without review too. So I'll go ahead and merge this. I'll make a working version of the multiple NaCl runtimes concept in a bit. |
|
So you just decided to break pending pull requests and re-introduce bugs. |
Someone has already invested months of work in that effort. |
Thanks for that. This could have been worded more humbly: I'll redo the final touches to the multi-NaCl-runtime effort in a bit, if you don't mind! |
|
I will never recover the time I spent working on this. At least when I merged the first bits of that effort I invested in, I rebased all your work so that you wouldn't waste your time doing so. And I was also making sure your effort wouldn't be wasted. I see this is not reciprocal. |
Honestly I would prefer to do it myself, and observe the rule of developers with merge access merging their own code.
I'm sorry you feel that way. I'm happy to help by rebasing any remaining pieces if desired. Unfortunately some of the code here does have to go to waste, since it was based on the superfluous helper process (#1336). The only way to avoid redoing it would be to bring back the useless process. |
More fixing of the unreviewed code merged in #2018:
I will add a working version of armhf->box64 fallback later. I'm thinking to make the NaCl architecture an argument to
CreateNaClVM, loop over calling that with possible more than one value, and throw an exception (instead ofSys::Error) if a special failure case corresponding to architecture incompatibility is detected.