-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
Replace current C#-based shim with C version #3
Conversation
Hi @r15ch13, The build is failing when running a script you added that collects the release notes. |
@ScoopInstaller/maintainers Please create a develop branch when you have time
This weekend I am finally getting back from Swiss work trip and I will have time for proper review. @gerardog Are you familiar with code or you just copied it from https://github.com/71/scoop-better-shimexe? |
- original C# shim (shim.cs -> shim.exe) - new C shim (shim.c -> newshim.exe)
Done @Ash258. Note that the same build scripts fails on a different point on my repo, after collecting the artifacts
Sure, waiting for the develop branch to be created. If
I just copied it but It looks simple enough so I can give a try if something comes up. |
I can also help if there's a problem. We should also find a way to sync changes from https://github.com/71/scoop-better-shimexe to this repo if other changes are made to the source over time. |
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.
- Add proper documentation to all functions.
- Format everything as in scoop related projects.
- https://github.com/lukesampson/scoop/blob/master/.editorconfig
- Use K&R 0TBS code style
if($LastExitCode -ne 0) { $host.SetShouldExit($LastExitCode ) } | ||
|
||
# Import Visual Studio 2017 MsBuild Env Vars | ||
cmd.exe /c "call `"C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Auxiliary\Build\vcvars32.bat`" && set > %temp%\vcvars.txt" |
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.
Why external cmd??
} | ||
|
||
Write-Output 'Compiling shim.c ...' | ||
& cmd /c cl.exe /O1 /Fe"$build\newshim.exe" "$src\shim.c" '2>&1' |
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.
Why external cmd??
Closing, as I believe scoop team is handling this now. |
As stated in ScoopInstaller/Scoop#3634,
The current C# based implementation can be replaced by a faster and better solution in C, proposed by @rasa. Its readme explains the benefits. Also closes several issues mentioned in ScoopInstaller/Scoop#3634.
This PR uses @rasa code and updates the appveyor build scripts. The build scripts can be improved for sure. Sorry it was my first use of AppVeyor.