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

chore: remove useless compiler_flag and copy .exe on windows #108

Merged
merged 1 commit into from
Jun 14, 2024

Conversation

XiaoPangxie732
Copy link
Contributor

v install should not be concerned with compilation flags.
And the actual compiler is determined by build.vsh:

PS C:\Users\***\.config\v-analyzer> $Env:CC = 'clang'
PS C:\Users\***\.config\v-analyzer> v .\install.vsh up --nightly
Installing latest nightly version...
Updating v-analyzer sources...
✓ Successfully updated v-analyzer sources
Building v-analyzer...
✓ Dependencies for v-analyzer installed successfully
Building v-analyzer at commit: 7e11a6f, build time: 2024-06-15 01:17:19 ...
✓ Prepared output directory
Building v-analyzer in debug mode, using: "E:\Programs\v\v.exe" "C:\Users\***\.config\v-analyzer\sources" -o "./bin/v-analyzer.exe" -no-parallel -cc clang  -g
To build in release mode, run v build.vsh release
Release mode is recommended for production use. At runtime, it is about 30-40% faster than debug mode.
✓ Successfully built v-analyzer!
Binary is located at C:\Users\***\.config\v-analyzer\sources\bin\v-analyzer.exe

Moving v-analyzer binary to the standard location...
Failed to copy v-analyzer binary to C:\Users\***\.config\v-analyzer\bin: Failed to remove "C:\Users\***\.config\v-analyzer\bin\v-analyzer.exe": Permission denied; code: 13
✓ v-analyzer successfully updated to nightly (7e11a6f8f369df935664fadd2f0c99d90fe3226f)
Path to the binary: C:\Users\***\.config\v-analyzer\bin\v-analyzer

Also, the executable has .exe extension on Windows, thus v-analyzer.exe should be copied on Windows

Copy link
Member

@spytheman spytheman left a comment

Choose a reason for hiding this comment

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

Excellent work.

@spytheman
Copy link
Member

The analyzer test failure is unrelated, and due to a change in string.is_upper() that happened in vlang/v#21358 , while v-analyzer's CI was not ran after that (we only test that v-analyzer itself builds on the main V CI, and not that v-analyzer's CI passes).

@spytheman
Copy link
Member

I am not sure whether to fix pascal_case_to_snake_case or its tests, but I am more inclined to change pascal_case_to_snake_case to preserve its old behavior. @ttytm what do you think?

@ttytm
Copy link
Member

ttytm commented Jun 14, 2024

if c.ascii_str().is_upper() {

- if c.ascii_str().is_upper() {
+ if c.ascii_str().is_upper() || c.is_digit() {

@ttytm
Copy link
Member

ttytm commented Jun 14, 2024

Would make it pass with the current utils tests. Additionally including a check of the last letter would make it more safe.

@spytheman
Copy link
Member

if c.ascii_str().is_upper() {

- if c.ascii_str().is_upper() {
+ if c.ascii_str().is_upper() || c.is_digit() {

Applied in 2584e86 .

@spytheman
Copy link
Member

(rebased over current main)

@spytheman spytheman merged commit 359cc2c into vlang:main Jun 14, 2024
15 checks passed
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