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

tools, ci: print info to use v symlink instead of v symlink -githubci #21471

Merged
merged 3 commits into from
May 8, 2024

Conversation

ttytm
Copy link
Member

@ttytm ttytm commented May 8, 2024

This one contains an interesting coincidence and revelation.

The last update used ./v symlink --githubci (double-dashed) instead of ./v symlink -githubci to cover extended cases for the flag. However, the double-dashed flag results in the default symlink being used instead of the GITHUB_PATH-symlink that is based on the current directory. Now, using the symlink with the single dash flag -githubci fails to pass the extended test cases.

This shows that using the flag actually results in worse functionality. The solution would be to already use setup_symlink instead of setup_symlink_github (even when the flag is passed), as it fixes an issue.

Based on the recent heated(while still constructive) discussion, the coincidence might look like an evil plan, setting this revelation up, but it really was just a tiny mistake that happened in the last PR.

@ttytm
Copy link
Member Author

ttytm commented May 8, 2024

I haven't made any fixes yet, the CI will fail at first. But if I may, I would make that internal update already (to always the same symlink) instead of awaiting a deprecation period and just warn about the flag being obsolete while keeping it functioning.

@spytheman
Copy link
Member

This should stay for several months:

if '-githubci' in os.args {
    setup_symlink_github()
}

Note, also that the CI passed fine in 279405a , which did use a single -, not a double one, and which did use v and not ./v . The only restriction is that you can not make && ./v symlink -githubci && v version in the same script, since that script will be executed in a context that has its PATH already set by the runner, and the change in the file, pointed by GITHUB_PATH, will become available only the scripts after it.

I've explained to you multiple times already, both here and on Discord, that I want to keep this explicit command: v symlink -githubci, working as it is, until the very last moment (months from now), and yet you repeatedly make the same change, over and over again, that I do not want.

Changes that affect v symlink --githubci, and v symlink, are fine.

I do not want to affect CIs that do use v symlink -githubci at all for now, that specific variant, should continue to do what it does currently.

I do not know how else to explain my position.

I accept, that I will look as stubborn ass to you, but so be it :-| .

@ttytm
Copy link
Member Author

ttytm commented May 8, 2024

It's fine for me to keep that as it is. Also it was understood and I didn't intended to remove it I didn't removed it here. Just seeing that it is failing with the flag where it isn't without it appeared like a selling point.

I'd update the CI test then to test the flag to its current capabilities. Adding the warning is fine though? As said no functionalities changed.

@ttytm
Copy link
Member Author

ttytm commented May 8, 2024

A selling point not to remove it, but to extend it's capabilities while still keeping it is what I meant.

@spytheman
Copy link
Member

Adding a warning is fine, as long, as it does not affect the actual functionality.

@spytheman
Copy link
Member

A selling point not to remove it, but to extend it's capabilities while still keeping it is what I meant.

That can be done too in the future, but not now. v symlink -githubci currently does not make a symlink on unix (which needs write permissions to /usr/local/bin, which in turn needs sudo, unless /usr/local/bin is owned by the current user, or is otherwise writable). It also works consistently across all platforms.

This has known limitations, but also known advantages too.

I do not want to complect what it does with creating a symlink for now. It is a very deliberate choice, that reduces the risk of breaking things, that currently work, and that provides a way to quickly go back to using v symlink -githubci, should the need arise, for a few months.
You may disagree with it, but I do insist that its action should stay the same for now.

@ttytm
Copy link
Member Author

ttytm commented May 8, 2024

Yes that's totally fine. I'm already onto supporting this decision by trying to increase coverage for the cases where it is used.

Just to have it mentioned:
A thing that could be done, running both functions if using the -githubci flag. Making sure that setup_symlink won't cause a failure and setup_symlink_github is always called. Else just keeping it as it is for the flag. Any preference?

@spytheman
Copy link
Member

A thing that could be done, running both functions if using the -githubci flag. Making sure that setup_symlink won't cause a failure and setup_symlink_github is always called. Else just keeping it as it is for the flag. Any preference?

I already stated my preference, which is that it should stay exactly as it is for now.
I thought, that I was clear about that?

@ttytm
Copy link
Member Author

ttytm commented May 8, 2024

I already stated my preference, which is that it should stay exactly as it is for now.
I thought, that I was clear about that?

💯

As said no issues with that, and supporting it. Still wanted to mention something that might not have been thought about. Thanks for the fix 👍

@spytheman spytheman merged commit 972a137 into vlang:master May 8, 2024
28 checks passed
@ttytm ttytm deleted the ci/symlink-info branch May 8, 2024 14:29
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.

None yet

2 participants