Eliminate the symlinks in the project

[krader1961]

I added an elint subcommand to my vm command that runs make all-checks on each VM that I use for exploring/verifying changes to Elvish (as well as other projects). The make all-checks fails on Windows because the vscode\LICENSE file is no longer a symlink (I'm using the rsync command to copy the project to Windows). This causes Git to report this:

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        typechange: vscode/LICENSE

Which causes an unexpected error from make all-checks on Windows.

Ideally there would be no symlinks in the project since they are not portable to non-Unix systems. However, telling Git to assume the symlink is unchanging via git update-index --assume-unchanged may be an acceptable workaround. I have verified that resolves this issue. That is, I ran this on my macOS server:

elvish> git update-index --assume-unchanged vscode/LICENSE

I then rsync'd the project to Windows and verified that git status no longer reported a "typechange" for that file. However, I could not create a Git commit that only includes that change to the project. I'm hoping that if @xiaq executes that command on the master branch and (force-)pushes the branch to Github that will then be picked up by all other developers. Alternatively, just replace the symlink with an actual copy of the license or replace the symlink with text that explicitly refers to the official license via a URL.

P.S., There is a second, optional, symlink, ./.git/hooks/pre-push. Its presence doesn't cause any problems when syncing the project to a Windows system. It could theoretically cause problems if someone working on a Windows system modified the linked to ./tools/pre-push script without modifying the copy "linked to" by ./.git/hooks/pre-push that is effectively a non-problem. Too, given the special nature of the ./.git directory there is no practical way to eliminate that, optional, symlink AFAICT. At least not without requiring the user to copy the ./tools/pre-push script to ./.git/hooks/pre-push. Which has its own version skew problems.

[krader1961]

Also, for the record, since I am scripting all of this I simply added this to the script I use to rsync the Elvish project to my VM's:

git update-index --assume-unchanged vscode/LICENSE

That "fixed" the unexpected "make all-checks" failure on my Windows VM. However, executing that command on my VM's shouldn't be necessary.

[krader1961]

Also, automating running the lint tests revealed that make all-checks on Windows results in this error:

  staticcheck ./...
  pkg\sys\eunix\termios_32bitflag.go:5:6: type termiosFlag is unused (U1000)

I'll open a separate issue and pull-request to address that lint error on Windows.

[krader1961]

Also, as I have mentioned in previous issues, and pull-requests, checking code correctness (i.e., "linting" the code) should output nothing if the code has no problems. As oppossed to the current behavior. This is from running make all-checks on my macOS system on the Elvish master branch as I write this:

./tools/check-fmt-go.sh
Go files need these changes:
  None!
./tools/check-fmt-md.sh
Markdown files that need changes:
  None!
./tools/check-disallowed.sh
codespell
go vet ./...
staticcheck ./...
./tools/check-gen.sh

In the example above I don't care about any of its output since it does not tell me there is a lint problem. I only care if the lint checks produce an error. I do not care about executing a check that does not identify a problem.

[xiaq]

I'll replace vscode/LICENSE with a simple file. The symlink you have in .git is not part of the repository.

I am aware of the classical "rule of silence", but given that go test already produces a lot of outputs (one line for each package without test errors), making the lint checks obey the rule of silence now would be inconsistent and also not really make any hook outputs less verbose.

[krader1961]

I am aware of the classical "rule of silence", but given that go test already produces a lot of outputs (one line for each package without test errors), making the lint checks obey the rule of silence now would be inconsistent and also not really make any hook outputs less verbose.

Okay, we'll have to agree to disagree. :-) What about adding an env var that tells the linters to be silent if no problems are found? The default being the current behavior. That would only slightly complicate the existing linting scripts.

Suppressing the output of a "clean" lint run is especially useful when I do something like this:

elvish> vm elint
Running sync-elvish...
fedora28    0.476 seconds (1.0x)
ubuntu32    0.479 seconds (1.0x)
fedora37    0.479 seconds (1.0x)
osuse       0.479 seconds (1.0x)
ubuntu64    0.583 seconds (1.2x)
freebsd     0.586 seconds (1.0x)
msys        2.417 seconds (4.1x)
Linting Elvish...
macpro      2.713 seconds (1.0x)
fedora37    2.923 seconds (1.1x)
fedora28   13.817 seconds (4.7x)
freebsd    13.933 seconds (1.0x)
ubuntu64   14.155 seconds (1.0x)
ubuntu32   14.266 seconds (1.0x)
osuse      14.731 seconds (1.0x)
msys       23.977 seconds (1.6x)
  ./tools/check-fmt-go.sh
  Go files need these changes:
    None!
  ./tools/check-fmt-md.sh
  Markdown files that need changes:
    None!
  ./tools/check-disallowed.sh
  codespell
  go vet ./...
  staticcheck ./...
  pkg\sys\eunix\termios_32bitflag.go:5:6: type termiosFlag is unused (U1000)
  make: *** [Makefile:41: most-checks] Error 1
  Exception: make exited with 2
  [tty 109]:1:1: make all-checks

I've modified my vm program to explicitly check for the following text in the make all-checks output and elide it from its output to keep the output as small and easy to scan by a human, for actual problems, as possible (as exhibited by the above transcript):

Go files need these changes:
  None!
./tools/check-fmt-md.sh
Markdown files that need changes:
  None!
./tools/check-disallowed.sh
codespell
go vet ./...
staticcheck ./...
./tools/check-gen.sh

This is fragile and high-maintenance. I'd prefer to eliminate that text constant in my vm program and just rely on make all-checks producing no output if zero problems are found. Also, note that the one system which reports an actual problem includes a lot of verbiage that is basically noise since it says everything verified by those checks was okay.

[krader1961]

Also, I'll note that I believe the comparison to go test ./... behavior is questionable. In fact, I depend on the current behavior of reporting successful test executions to provide detailed output regarding the number of tests executed and passed or failed:

elvish> vm etest
Running sync-elvish...
fedora37    0.471 seconds (1.0x)
osuse       0.474 seconds (1.0x)
fedora28    0.583 seconds (1.2x)
ubuntu64    0.585 seconds (1.0x)
ubuntu32    0.586 seconds (1.0x)
freebsd     0.699 seconds (1.2x)
msys        2.406 seconds (3.4x)
Running Elvish unit tests...
macpro     15.045 seconds (1.0x)
fedora37   26.772 seconds (1.8x)
ubuntu32   82.506 seconds (3.1x)
fedora28  146.446 seconds (1.8x)
osuse     148.999 seconds (1.0x)
ubuntu64  152.157 seconds (1.0x)
msys      168.118 seconds (1.1x)
freebsd   201.401 seconds (1.2x)

macpro     57 ok    0 fail   27 not run
osuse      57 ok    0 fail   27 not run
fedora28   57 ok    0 fail   27 not run
fedora37   57 ok    0 fail   27 not run
ubuntu64   57 ok    0 fail   27 not run
ubuntu32   57 ok    0 fail   27 not run
freebsd    57 ok    0 fail   27 not run
msys       55 ok    0 fail   31 not run

[xiaq]

Also, I'll note that I believe the comparison to go test ./... behavior is questionable. In fact, I depend on the current behavior of reporting successful test executions to provide detailed output regarding the number of tests executed and passed or failed:

So you're already redirecting and processing the outputs of go test - you could do the same for the linting script?

Maybe the real issue here is that the output of the linting script should be made easier to parse? That's something I cam get behind.