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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CurrentDirectory to JclSysUtils.Execute #79

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

silverqx
Copy link

The CurrentDirectory parameter is passed down to the CreateProcess Windows function as the lpCurrentDirectory parameter.
I decided to add this parameter as the last parameter to all JclSysUtils.Execute functions with a default value '' ( empty string ).
I added PCurrentDirectory: PChar inside JclSysUtils.ExecuteCmdProcess function, because if CurrentDirectory is empty string, nil have to be passed down to the CreateProcess, in this case, the new process will have the same current drive and directory as the calling process.
This PR is backward compatible, doesn't contain any breaking changes. 馃

@obones
Copy link
Member

obones commented Feb 24, 2020

Hello,

Thank you for this pull request. As I read it, the code changes are only valid for a Windows target. This means that the new function parameter has no impact when compiling for Linux (with FPC) and thus gives a false sense that it would serve a purpose.
For Windows specific options, you must limit yourselves to adding a new field to the IFDEF section of TJclExecuteCmdProcessOptions (see 267c312 for an example)
If, however, you find a valid way to perform the same functionality for all targerted platforms, then the new parameter will make sense

@silverqx
Copy link
Author

You are right, I did it this way, but I changed it right before I sent this PR 馃, I don't even why. I change it back, like you are proposing and send an update later.

@silverqx silverqx changed the title Added CurrentDirectory to JclSysUtils.Execute Add CurrentDirectory to JclSysUtils.Execute Feb 25, 2020
CurrentDirectory parameter is passed to the CreateProcess
Windows function
@silverqx
Copy link
Author

Done, I moved FCurrentDirectory field and CurrentDirectory property inside {$IFDEF MSWINDOWS} directive.

@obones
Copy link
Member

obones commented Feb 26, 2020

Thanks for that, but this does not addresss my remark about the new parameter you added to all functions. With the current code, it won't compile for LINUX. Sure, you could add a IFDEF block around the CurrentDirectory assignment, but it won't change the fact that the new parameter has no effect when compiled for LINUX.

@silverqx
Copy link
Author

silverqx commented Feb 26, 2020

It will compile in Linux env. without any problems, the parameter has default value ='' and it value will not be used in Linux env., the same is true for other parameters like AutoConvertOem, AbortEvent, AbortPtr, ProcessPriority, all this and maybe I missed someone, are not used in Linux env., I don't think that wrap CurrentDirectory parameter into {$IFDEF MSWINDOWS} directive is a good idea, it could make problems in future. 馃槓

@silverqx
Copy link
Author

If you look at all Execute function declarations, you will see, that all last parameters are only windows specific, before CurrentDirectory are always ProcessPriority and AutoConvertOem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants