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: make ./v symlink work platform independent in CI #21453

Merged
merged 3 commits into from
May 7, 2024

Conversation

ttytm
Copy link
Member

@ttytm ttytm commented May 7, 2024

Splits up changes to make ./v symlink work platform independent in CI.

The PR:

  • Moves handling of github symlink to the vsymlink_windows.c.v code. On unix-like systems regular symlinking will be used, whether or not -githubci was passed.
  • An improvement made alongside is that instead of adding os.getwd() to the GITHUB_PATH, new_sys_env_path is used. It is the path used on a regular windows systems -> Brings tests in Windows CI closer to a regular scenario, makes them more reliable.
  • Now ./v symlink should already work platform independent. The -githubci flag is not required anymore. Cases with sudo and the -githubci flag are still covered by the current, unchanged CI jobs. They should continue to work and there are no plans in changing this inside the PRs that are implementing this enhancement. Only adding an info that the flag is not required anymore.

@ttytm ttytm changed the title tools: improve environment detection when creating symlinks p1 tools: make ./v symlink work platform independent in CI May 7, 2024
@spytheman
Copy link
Member

spytheman commented May 7, 2024

-githubci should always invoke setup_symlink_github, and that setup_symlink_github should stay just as it is for now, in the vsymlink.v.

i.e. that is the last thing that should be changed, after we are sure that v symlink behaves well consistently in all situations on the CI.

@spytheman
Copy link
Member

new_sys_env_path can be added in addition to the rest, if it is needed on windows.

@spytheman
Copy link
Member

Now ./v symlink should already work platform independent. The -githubci flag is not required anymore. Cases with sudo and the -githubci flag are still covered by the current, unchanged CI jobs. They should continue to work and there are no plans in changing this inside the PRs that are implementing this enhancement. Only adding an info that the flag is not required anymore.

That will be nice, if true. I am not confident in it.
I want the behavior of v symlink -githubci to stay the same, until it is no longer needed. The point of doing things in steps, instead of in one big change, is to minimize/spread the risk of breakages of CIs that already do use v symlink -githubci . It is not just to make the diff smaller, while containing the exact same modifications, to which I already objected once.

@ttytm
Copy link
Member Author

ttytm commented May 7, 2024

The goal is to make the it work without the flag. And it works without the flag.
It should also be made sure that it continue to works with the flag. This is proven to work by the passed CI jobs that still use -githubci flags even when the flags don't do anything.

As it is a simple change idk what is wrong. Trying to understand the problem if you would share what is not tested here, it would help.

In the theoretical case of scenarios where nix systems would require V in the GITHUB_PATH it could still be done automatically in CI jobs - similar to how the update does it on Windows. This automation could be made in a testable way. From all available infront of us, regular symlinking on nix systems is sufficient. If there should be an issue a fix will be simple.

@ttytm
Copy link
Member Author

ttytm commented May 7, 2024

Too small steps for me, considering the relation to time, chance of breakage and easy possibilities to fix. The request of making the change of removing the -githubci flag from other runners to have this new functionality tested in a separate PR makes the changes in this PR untested, which makes it for me personally illogical and a less safe step.

@spytheman
Copy link
Member

spytheman commented May 7, 2024

This is proven to work by the passed CI jobs that still use -githubci flags even when the flags don't do anything.

Many of the CI jobs do use ./v or the equivalent .\v.exe inside them, which will work even without any v symlink at all, instead of v, which will break, if v is not on the PATH.

They are written like that, in part because of how flaky v symlink and v symlink -githubci were historically.

@spytheman
Copy link
Member

spytheman commented May 7, 2024

From all available infront of us, regular symlinking on nix systems is sufficient.

Symlinking to /usr/local/bin, requires that you have write permissions to that folder.

Writing to a file, whose location is described in GITHUB_PATH, when it is set, as described by github's docs, does not require any additional write permissions for your runner, and will continue to work, no matter what happens with their runners, and images. It also works consistently no matter if the platform is unix or windows. I can not stress this enough.

v symlink -githubci takes advantage of that, and does so in a consistent manner, that I want to preserve as much as possible, while v symlink behavior evolves as you wish.

Once v symlink works just as consistently as v symlink -githubci on the CI, which can be checked, by running all CI for a few days, after changing the ./v in it to v, we can add a warning for v symlink -githubci, while still preserving its consistent current behavior.

After several months, all other CIs for V projects will have had time to upgrade, and we can remove the behavior for v symlink -githubci, not before.

@spytheman
Copy link
Member

The request of making the change of removing the -githubci flag from other runners to have this new functionality tested in a separate PR makes the changes in this PR untested, which makes it for me personally illogical and a less safe step.

I did not object to removing the -githubci flag from other runners.

I do strongly object to removing the functionality of passing -githubci in the source code of the last and in the current PR.

@spytheman
Copy link
Member

The current state, after e012ca8 is fine.

@ttytm
Copy link
Member Author

ttytm commented May 7, 2024

Symlinking to /usr/local/bin, requires that you have write permissions to that folder.

That is true but of relevance for local symlinking. In GitHub CI the folder should have write permissions in the same way as it is not required to add credentials when running commands as sudo. When those default permissions are changed it's a custom case where the one doing those permission changes will need to take care of it.

I'll try that patience thing when making the smallest possible steps so that data can better cover things than words can.

@spytheman spytheman merged commit 6cda618 into vlang:master May 7, 2024
29 checks passed
@ttytm ttytm deleted the tools-ci/symlink-1 branch May 7, 2024 16:30
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