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

Implement zigup default master #34

Merged
merged 3 commits into from Jan 7, 2022

Conversation

iddev5
Copy link
Contributor

@iddev5 iddev5 commented Nov 13, 2021

It just removes the explicit check as missing of master symlink would already get detected by setDefaultCompiler().
Implements as per discussed on discord/issue topic.

I noticed that you aren't doing zig fmt, so I didn't did it either.

Closes #33

zigup.zig Show resolved Hide resolved
@iddev5 iddev5 requested a review from marler8997 January 2, 2022 12:50
@iddev5
Copy link
Contributor Author

iddev5 commented Jan 2, 2022

I have also rebased it to master

} else {
try setDefaultCompiler(allocator, compiler_dir, .verify_existence);
}
try setDefaultCompiler(allocator, compiler_dir, .verify_existence);
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this will work on windows since it doesn't have symlinks (not sure why github actions isn't testing your PR). I'm fairly certain that what needs to be done is when we see "master", we need to resolve the master version (if master has not been downloaded print a nice error that it hasn't been fetched), and then call setDefaultCompiler with that specific version.

Copy link
Owner

Choose a reason for hiding this comment

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

Also we should test the case where we run zigup deafult master before we have downloaded anything and verify we get a good error message.

If you want me to finish the PR I can but I'll give you a change to implement it if you like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh alright. I don't have a windows PC so I couldn't test it and thus I cannot reliably implement it either. So yeah please continue.

@marler8997
Copy link
Owner

Ok I finished the implementation and added a test to make sure that we get a good error when master is not fetched. I'll give you a change to review the code before merging.

@iddev5
Copy link
Contributor Author

iddev5 commented Jan 7, 2022

Ohh it makes sense now, looks good

@marler8997 marler8997 merged commit b4b7d2a into marler8997:master Jan 7, 2022
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.

zig default master
2 participants