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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the settings command for OSes other than Windows #973

Merged
merged 3 commits into from
Oct 29, 2022

Conversation

abheekda1
Copy link
Contributor

@abheekda1 abheekda1 requested a review from rfuzzo October 21, 2022 20:15
@abheekda1 abheekda1 linked an issue Oct 21, 2022 that may be closed by this pull request
@abheekda1 abheekda1 requested a review from njibhu October 21, 2022 23:35
@abheekda1
Copy link
Contributor Author

@njibhu I am unable to do so right now but if you would like to go check out whether it works both with EDITOR set and with it unset that would be great.

Copy link
Contributor

@njibhu njibhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When trying it out I get:

Opening /mnt/javik-data/Workspace/WolvenKit/x64/appsettings.json
$EDITOR not set, please set it if you would like to open the settings in a CLI text editor.
Unhandled exception: System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
 ---> System.ComponentModel.Win32Exception (13): An error occurred trying to start process '/mnt/javik-data/Workspace/WolvenKit/x64/appsettings.json' with working directory '/mnt/javik-data/Workspace/WolvenKit'. Permission denied
   at System.Diagnostics.Process.ForkAndExecProcess(ProcessStartInfo startInfo, String resolvedFilename, String[] argv, String[] envp, String cwd, Boolean setCredentials, UInt32 userId, UInt32 groupId, UInt32[] groups, Int32& stdinFd, Int32& stdoutFd, Int32& stderrFd, Boolean usesTerminal, Boolean throwOnNoExec)
   at System.Diagnostics.Process.StartCore(ProcessStartInfo startInfo)
   at System.Diagnostics.Process.Start()
   at CP77Tools.Commands.SettingsCommand.Action(IHost host) in /mnt/javik-data/Workspace/WolvenKit/WolvenKit.CLI/Commands/SettingsCommand.cs:line 53
   --- End of inner exception stack trace ---
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Span`1& arguments, Signature sig, Boolean constructor, Boolean wrapExceptions)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Delegate.DynamicInvokeImpl(Object[] args)
   at System.Delegate.DynamicInvoke(Object[] args)
   at System.CommandLine.NamingConventionBinder.ModelBindingCommandHandler.InvokeAsync(InvocationContext context)
   at System.CommandLine.NamingConventionBinder.ModelBindingCommandHandler.Invoke(InvocationContext context)
   at CP77Tools.Commands.CommandBase.ActionBase(InvocationContext context) in /mnt/javik-data/Workspace/WolvenKit/WolvenKit.CLI/Commands/CommandBase.cs:line 26
   at System.CommandLine.Invocation.AnonymousCommandHandler.InvokeAsync(InvocationContext context)
   at System.CommandLine.Invocation.AnonymousCommandHandler.SyncUsingAsync(InvocationContext context)
   at System.CommandLine.Invocation.AnonymousCommandHandler.Invoke(InvocationContext context)
   at System.CommandLine.Invocation.InvocationPipeline.<>c__DisplayClass4_0.<<BuildInvocationChain>b__0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass17_0.<<UseParseErrorReporting>b__0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Hosting.HostingExtensions.<>c__DisplayClass1_0.<<UseHost>b__0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass12_0.<<UseHelp>b__0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass22_0.<<UseVersionOption>b__0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass19_0.<<UseTypoCorrections>b__0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c.<<UseSuggestDirective>b__18_0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass16_0.<<UseParseDirective>b__0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c.<<RegisterWithDotnetSuggest>b__5_0>d.MoveNext()
--- End of stack trace from previous location ---
   at System.CommandLine.Builder.CommandLineBuilderExtensions.<>c__DisplayClass8_0.<<UseExceptionHandler>b__0>d.MoveNext()

@njibhu
Copy link
Contributor

njibhu commented Oct 22, 2022

And with the EDITOR variable setup, it works properly

@abheekda1
Copy link
Contributor Author

Ok so EDITOR works good, but just opening it doesn't. Do you think you could try compiling with UseShellExecute set to true and maybe removing the CreateNoWindow param?

@njibhu
Copy link
Contributor

njibhu commented Oct 24, 2022

CreateNoWindow already doesn't exist, so I'm not sure what you mean.
With UseShellExecute = true it does work the same as with xdg-open

@abheekda1
Copy link
Contributor Author

Sorry, you're right, I think that's what I meant. Now I think it should all work great 🤞🏽.

@abheekda1 abheekda1 requested a review from njibhu October 24, 2022 22:35
@abheekda1
Copy link
Contributor Author

@rfuzzo can I just merge this?

@abheekda1 abheekda1 closed this Oct 28, 2022
@abheekda1 abheekda1 reopened this Oct 28, 2022
@rfuzzo rfuzzo merged commit 432d166 into main Oct 29, 2022
@rfuzzo rfuzzo deleted the fix/linux-cli-settings branch October 31, 2022 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI settings exception when headless on Linux
3 participants