The @open application passes paths without quotes #336
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The
@openapplication passes paths without quotes, which means that if there are spaces in the file path, the argument parser in C programs treats them as separate arguments. This behavior breaksDGEN. It is necessary to come up with an optimal solution so as not to break programs that do not understand quotes.Один из вариантов решения проблемы: стандартизировать передачу аргументов командной строки. Если так, то необходимо провести следующее:
There is an option "kill two birds with one stone": if there are spaces in a string, put them in quotation marks; if there aren't, leave them as is.
@mxlgv Hey there!
I’ve been looking into the issue with @open passing paths without quotes and have locally implemented a solution inside DGen. Since this would be my first contribution to kolibrios, I'd love some guidance on the workflow. Should I fork the github mirror to submit the PR since this repo is read only?
The fix is essentially a loop before assigning the path name to the rom variable. It checks if a file with the current name actually exists, if not, it keeps appending the next argument until it actually finds a valid file.
I know the bigger plan from @Doczom is the ideal long term solution, but since this is mostly a big standalone project, I though about making a direct hotfix for now.
I am really open to discussion (I even thought about an initial plan for the long solution), so please let me know if we should continue here, on Discord, or the GitHub mirror, and if you have any additional comments.
@mxlgv I made my fix and posted my previous comment before reading this comment of yours sorry.
Regarding the suggestion that was my first thought too but I think this would still break almost all other assembly code out there since (I guess) currently the assembly files don't think about the quotes at all, to fix it we would need to do the big solution doczom suggested.
I suggest adding the following section of documentation and compiling a list of programs subject to the changes.
#215 (comment)
@Doczom I like what you described. And I have a feeling that such a change within
@openwon't break anything, since this program is needed to open a file, and it only takes one argument - the path.@Matou1306 Github is RO mirror. We don't accept pull requests there. We also have CI on Gitea. So you'll have to create a fork and then make a PR.
PS: I think I saw your PR on GitHub. Unfortunately, it doesn't look like a real fix yet, more like a workaround.
@mxlgv Just to clarify, I didn't open any PR on github, it's mostly someone else. I can't find it so I can't compare it to my solution, but the explanation for mine is above.
Also I know the fix isn't a full one. I went with that method because I thought the full solution would take ages to implement across the board. My concern was that changing the format globally might cause more bugs if merged before every program is ready for it (please let me know if this concern is valid).
But since the documentation for the new standard is being drafted now, I’d rather just do it the "right" way for DGen as a first step. I'm down to implement the quoting/escaping rules directly instead of the workaround, I just don't want to overcomplicate things if it's not needed yet.
Let me know if you think I should go ahead with that for DGen specifically or if you have other guidance for me to start right ahead.
@Matou1306 I see... Sorry, so it wasn't your PR. I think we should solve the
@openissue and not come up with workarounds forDGENand similar programs.@mxlgv I opened #344 with the implementation we discussed.
I believe this covers the standardization method @Doczom suggested (quoting in @open then handling it in native apps).
Please let me know if I need to adjust anything regarding the commit/code style since it's my first contribution (I tried my best to stay consistent though) or if I missed anything in the solution.
@Doczom
Something tells me that we chose the wrong strategy. It didn’t dawn on me right away, but it would break everything! I also assume that the problem is not only with
@openbut, for example, with the “open with” selection dialog in Eolite. I'm not sure there's any reason to fix every program.@Matou1306
I recommend temporarily doing nothing on this task until we come to something more suitable.
This is actually an architectural problem with the command line and the way it is used. If I remember correctly, only C programs suffer from this. Now I can’t find a better solution than writing a template QUOTKINIZER).
This will be a small proxy program on FASM. Like the one that loads video drivers.
For example: next to the
dgenbinary there will bedgen_openwhich will add quotes to the path.In the
assoc.inifile we can specify wrappers instead of the original files for problematic programs.If the user is using the wrong binary, that's their problem.
@Matou1306
Honestly, I have to apologize for approving this decision and for you doing extra work.
@mxlgv I understand that your architectual decision has changed and that you think that the global fix is too risky right now
I have spent the last few days digging into the codebase to implement the solution exactly as @Doczom proposed, specifically to prepare for GSoC.
Could you please review the PR or explain how this could break other workflows like Eolite? I’d love to understand the system better and see if there is still a safe way to close this issue.
If this task is strictly on hold, could you at least point me to a different bug that is confirmed "ready for a fix"? (I actually went through the issues list and this one looked the most 'ready', so I'm asking for your recommendation to avoid hitting a dead end again)
I am serious about contributing and would love to get involved.
@Matou1306
The problem is that there are quite a lot of programs that could potentially not work with quotes. See here: https://git.kolibrios.org/KolibriOS/kolibrios/src/branch/main/data/common/settings/assoc.ini
You should understand how Oberon programs, C--, C (TCC + GCC) handle quotes on the command line. In addition, in the affected FASM programs it is necessary to write additional processing of these quotes. The macro that you wrote is possible and a working solution, but it is clearly not optimal, at least in terms of size.
I believe that the correct solution would be the POSIX solution: to delegate the parsing of arguments to the shell. In our case,
@open,SHELL, etc. and pass them as arguments toexecv()- like function. But this is not possible now.This is why I believe that now it is faster and easier to write wrappers for problematic programs than to create a problem for all programs.
@mxlgv Thanks for the detailed explanation. From what I understand you are suggesting we should go back to tailoring a fix for dgen specifically right?
I had already thought of such a method so I just implemented it and updated the PR.
The Logic: It loops through the arguments starting from the first non-option one. It treats the first argument as a potential filename, checks if it exists using access(). If not, it appends the next argument (adding a space in between) and checks again, repeating until a valid file is found or arguments run out.
I made sure to use minimal time and space for that local fix, please take a look and I am here waiting for your review or to make any additional changes for dgen or expand to other apps