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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

tools: skip download count increment when vpm runs in CI by default #19861

Merged
merged 2 commits into from Nov 15, 2023

Conversation

ttytm
Copy link
Member

@ttytm ttytm commented Nov 13, 2023

This PR would make VPM skip incrementing the download count of modules the deafult behavior when they are installed in CI.

To explicitly enable incrementing, VPM_NO_INCREMENT=0 can be set.

馃 Generated by Copilot at 34bbfc2

This pull request enhances the vpm tool by adding a feature to prevent incrementing the download count of modules when vpm runs in a CI environment or when VPM_NO_INCREMENT is set. It also updates the install_test.v file to test this feature.

馃 Generated by Copilot at 34bbfc2

  • Implement logic to prevent vpm from incrementing download count of modules in CI environment or if VPM_NO_INCREMENT is set (link, link, link)
  • Modify install_test.v to conditionally set VPM_NO_INCREMENT based on CI value (link)
  • Add no_inc_env variable to settings.v to store VPM_NO_INCREMENT value (link)
  • Use no_inc_env and CI value to set no_dl_count_increment field in Settings struct (link)

@spytheman
Copy link
Member

Why? There is no difference between running the tests on CI, or locally. In both cases, the incrementing should not be done => setting VPM_NO_INCREMENT unconditionally as it is on master is better.

@ttytm
Copy link
Member Author

ttytm commented Nov 13, 2023

In both cases, the incrementing should not be done

That's the reason for the PR.

Currently, when running v install ... in CI, vpm calls the increment function when VPM_NO_INCREMENT is not explicitly set. This increments numbers artificially. Since all CI runs whether on the V repo, forks or other projects that use v install will increment download counts. The change detects if it's a CI run and won't increment, without having to set VPM_NO_INCREMENT != ''.

VPM_NO_INCREMENT is for the vpm tests that can also run locally and for whatever case that wants to disable incrementing manually. It works as before with the exception that it can be set to 0 to explicitly enable incrementing. Unfortunately, adding a $if test condition inside vpm can't achieve an automatic detection for local tests like it's possible for CI where the CI env var is set automatically. Therefore, the VPM_NO_INCREMENT env var.

@ttytm
Copy link
Member Author

ttytm commented Nov 13, 2023

Or do you mean to remove the possibility to explicitly enable it?

@medvednikov
Copy link
Member

I think it's a good fix.

@spytheman
Copy link
Member

spytheman commented Nov 14, 2023

I often run ./v test-all or ./v test-self locally. These in turn do run v cmd/tools/vpm/install_test.v .
I do not want those commands, to trigger an update to the download counter (so perhaps they should also set VPM_NO_INCREMENT).
On master, install_test.v sets VPM_NO_INCREMENT, so that will not happen.

In this PR, setting VPM_NO_INCREMENT is conditional, and will not trigger by my local runs.
I object to that. I think that running v cmd/tools/vpm/install_test.v should always set VPM_NO_INCREMENT.

If I happen to run v install markdown, I do want to trigger the increment counter.

Imho a better solution will be, if you can check for CI in vpm, and set a flag if either VPM_NO_INCREMENT or CI is set.

@spytheman
Copy link
Member

i.e. I object to this:

if os.getenv('CI') == '' {
	os.setenv('VPM_NO_INCREMENT', '1', true)
}

It should not be conditional.

@ttytm
Copy link
Member Author

ttytm commented Nov 14, 2023

In this PR, setting VPM_NO_INCREMENT is conditional, and will not trigger by my local runs.

Admittedly, I don't really see the problem with the condition yet, as no increment will be set when CI is not set, which should be the case for local tests. You would have to set the CI env var in your config to increment downloads in the test. The test checks that the download count is skipped, also when run locally. If you run it locally does it fail?

@ttytm
Copy link
Member Author

ttytm commented Nov 14, 2023

Even when CI is set as env var locally, this will enable no increment. So you will have to set VPM_NO_INCREMENT=0 explicitly to get the undesired behaviour.

The condition was intended as test to have skipping tested when it's run in CI. Just in one of the test files seemed enough. But I also realise this test is shit, it doesn't test the behaviour in CI explicitly. Not-incrementing will be always set.

The main use case in the PR is to extend not incrementing downloads from vpms own little tests to to all v install runs if they are executed in a CI workflow. For the tests, things should stay as they are. Run locally, as well as in CI tests.

@ttytm
Copy link
Member Author

ttytm commented Nov 14, 2023

Were we both stupid or is it just me being an ape again? In either case, this is not something that should be on the internet 馃槅

@spytheman
Copy link
Member

spytheman commented Nov 14, 2023

Were we both stupid or is it just me being an ape again? In either case, this is not something that should be on the internet 馃槅

Oh, the internet usually does not care, and can benefit from more laughter.
I think my share of the stupidity may be bigger, since I'm often stupid and confident 馃槅.

In this case however, I really do think that VPM_NO_INCREMENT should be set unconditionally in the _test.v files, since it is simpler. The alternative is more brittle. Since that is a bit subjective though, it can be merged, if Alex wants it.

@spytheman
Copy link
Member

To explicitly enable incrementing, VPM_NO_INCREMENT=0 can be set.

That is also a bit bothering to me, since it is a double negation, and thus easier to get wrong. Imho VPM_NO_INCREMENT=1 is more explicit.

@ttytm ttytm force-pushed the tools.vpm/no-dl-increment-in-ci branch from 34bbfc2 to 220ad96 Compare November 14, 2023 17:36
@spytheman spytheman merged commit bc24683 into vlang:master Nov 15, 2023
42 checks passed
@ttytm ttytm deleted the tools.vpm/no-dl-increment-in-ci branch December 15, 2023 22:08
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

3 participants