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

Detect symlink creation capability on Windows #617

Merged

Conversation

HertzDevil
Copy link
Contributor

Explicitly detects the presence of Developer mode or the ability to enable SeCreateSymbolicLinkPrivilege, and errors immediately upon running any command that potentially fetches dependencies (build, install, lock, outdated, run, update). This reduces the chances of leaving the lib directory in an incomplete state in the middle of those commands.

The old exception handler for symlink failures is now gone; if an unhandled exception shows up, that means this PR is not exhaustive enough.

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

LGTM.

But it's quite a bit of very platform-specific code for stuff that a high-level app such as shards shouldn't really need to worry about.
I understand these helpers would be universally applicable and would most likely be useful for other Crystal programs. So perhaps we should move them to stdlib?

@@ -32,4 +63,64 @@ module Shards::Helpers
name
{% end %}
end

def self.privilege_enabled?(privilege_name : String) : Bool
{% if flag?(:win32) %}
Copy link
Contributor

@Sija Sija Feb 20, 2024

Choose a reason for hiding this comment

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

I'd suggest to put those (helpers) in a Windows namespace or mark them in other way (prefix, perhaps?) as windows-specific.

@HertzDevil
Copy link
Contributor Author

Moving things into stdlib (even Crystal::System) could break backward compatibility because previous versions of Crystal won't have the same helpers. In fact we might have inadvertently broken it before, and we should have some assurance that Crystal 1.0.0, or even 0.35.0 as suggested in Shards itself's shard.yml, is able to build Shards.

@straight-shoota
Copy link
Member

Windows is not fully supported anyway, so I wouldn't worry too much about it. We can still keep a copy of the code in shards repo to backfill for older Crystal versions. But ultimately, I don't think this should be part of the shards codebase.

@straight-shoota straight-shoota added this to the 0.18.0 milestone Feb 21, 2024
@straight-shoota straight-shoota merged commit 80b038a into crystal-lang:master Feb 22, 2024
7 checks passed
@HertzDevil HertzDevil deleted the feature/windows-symlink-check branch February 23, 2024 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants