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

crl-release-22.2: vfs: default to Unix semantics in MemFS #3000

Merged
merged 1 commit into from
Oct 17, 2023

Conversation

jbowens
Copy link
Collaborator

@jbowens jbowens commented Oct 16, 2023

Backport of #2065. Informs cockroachdb/cockroach#112087.


Default to Unix semantics in MemFS. The Windows semantics are left as configurable for unit tests that seek to ensure we don't remove open files, when possible.

There are existing cases where we cannot satisfy Windows semantics, and Go does not support opening files with the appropriate FILE_SHARE_DELETE permission bit (see golang/go#34681, golang/go#32088). The MemFS's implementation of Windows semantics is useful for ensuring we don't regress in the cases where we can satisfy Windows semantics.

Close #2064.
Informs #1236.

Default to Unix semantics in MemFS. The Windows semantics are left as
configurable for unit tests that seek to ensure we don't remove open
files, when possible.

There are existing cases where we cannot satisfy Windows semantics, and
Go does not support opening files with the appropriate
`FILE_SHARE_DELETE` permission bit (see golang/go#34681,
golang/go#32088). The MemFS's implementation of Windows semantics is
useful for ensuring we don't regress in the cases where we can satisfy
Windows semantics.

Close cockroachdb#2064.
Informs cockroachdb#1236.
@jbowens jbowens requested review from a team and sumeerbhola October 16, 2023 15:52
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@jbowens
Copy link
Collaborator Author

jbowens commented Oct 16, 2023

Windows build failed (and has been failing on crl-release-22.2 HEAD) with:

2023-10-16T16:05:40.0230279Z # github.com/cockroachdb/pebble/internal/cache.test
2023-10-16T16:05:40.0335546Z C:\hostedtoolcache\windows\go\1.18.10\x64\pkg\tool\windows_amd64\link.exe: running gcc failed: exit status 1
2023-10-16T16:05:40.0342402Z C:/ProgramData/Chocolatey/lib/mingw/tools/install/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\Users\RUNNER~1\AppData\Local\Temp\go-link-3311659118\000005.o: in function `_cgo_preinit_init':
2023-10-16T16:05:40.0347973Z \\_\_\runtime\cgo/gcc_libinit_windows.c:30: undefined reference to `__imp___iob_func'
2023-10-16T16:05:40.0351671Z C:/ProgramData/Chocolatey/lib/mingw/tools/install/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\Users\RUNNER~1\AppData\Local\Temp\go-link-3311659118\000005.o: in function `x_cgo_sys_thread_create':
2023-10-16T16:05:40.0357241Z \\_\_\runtime\cgo/gcc_libinit_windows.c:60: undefined reference to `__imp___iob_func'
2023-10-16T16:05:40.0367924Z C:/ProgramData/Chocolatey/lib/mingw/tools/install/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\Users\RUNNER~1\AppData\Local\Temp\go-link-3311659118\000005.o: in function `x_cgo_notify_runtime_init_done':
2023-10-16T16:05:40.0375064Z \\_\_\runtime\cgo/gcc_libinit_windows.c:101: undefined reference to `__imp___iob_func'
2023-10-16T16:05:40.0380608Z C:/ProgramData/Chocolatey/lib/mingw/tools/install/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\Users\RUNNER~1\AppData\Local\Temp\go-link-3311659118\000006.o: in function `x_cgo_thread_start':
2023-10-16T16:05:40.0532719Z \\_\_\runtime\cgo/gcc_util.c:18: undefined reference to `__imp___iob_func'
2023-10-16T16:05:40.0539696Z C:/ProgramData/Chocolatey/lib/mingw/tools/install/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\Users\RUNNER~1\AppData\Local\Temp\go-link-3311659118\000007.o: in function `_cgo_sys_thread_start':
2023-10-16T16:05:40.0545287Z \\_\_\runtime\cgo/gcc_windows_amd64.c:31: undefined reference to `__imp___iob_func'
2023-10-16T16:05:40.0548778Z collect2.exe: error: ld returned 1 exit status
2023-10-16T16:05:40.0588004Z 
2023-10-16T16:06:29.9697827Z # github.com/cockroachdb/pebble.test
2023-10-16T16:06:29.9699280Z C:\hostedtoolcache\windows\go\1.18.10\x64\pkg\tool\windows_amd64\link.exe: running gcc failed: exit status 1
2023-10-16T16:06:29.9703015Z C:/ProgramData/Chocolatey/lib/mingw/tools/install/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\Users\RUNNER~1\AppData\Local\Temp\go-link-726652978\000047.o: in function `_cgo_preinit_init':
2023-10-16T16:06:29.9705794Z \\_\_\runtime\cgo/gcc_libinit_windows.c:30: undefined reference to `__imp___iob_func'
2023-10-16T16:06:29.9708920Z C:/ProgramData/Chocolatey/lib/mingw/tools/install/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\Users\RUNNER~1\AppData\Local\Temp\go-link-726652978\000047.o: in function `x_cgo_sys_thread_create':
2023-10-16T16:06:29.9711632Z \\_\_\runtime\cgo/gcc_libinit_windows.c:60: undefined reference to `__imp___iob_func'
2023-10-16T16:06:29.9714822Z C:/ProgramData/Chocolatey/lib/mingw/tools/install/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\Users\RUNNER~1\AppData\Local\Temp\go-link-726652978\000047.o: in function `x_cgo_notify_runtime_init_done':
2023-10-16T16:06:29.9717619Z \\_\_\runtime\cgo/gcc_libinit_windows.c:101: undefined reference to `__imp___iob_func'
2023-10-16T16:06:29.9720651Z C:/ProgramData/Chocolatey/lib/mingw/tools/install/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\Users\RUNNER~1\AppData\Local\Temp\go-link-726652978\000048.o: in function `x_cgo_thread_start':
2023-10-16T16:06:29.9723233Z \\_\_\runtime\cgo/gcc_util.c:18: undefined reference to `__imp___iob_func'
2023-10-16T16:06:29.9726190Z C:/ProgramData/Chocolatey/lib/mingw/tools/install/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\Users\RUNNER~1\AppData\Local\Temp\go-link-726652978\000049.o: in function `_cgo_sys_thread_start':
2023-10-16T16:06:29.9728901Z \\_\_\runtime\cgo/gcc_windows_amd64.c:31: undefined reference to `__imp___iob_func'
2023-10-16T16:06:29.9730032Z collect2.exe: error: ld returned 1 exit status

@jbowens
Copy link
Collaborator Author

jbowens commented Oct 16, 2023

Investigating a fix to that separately in #3001.

@jbowens jbowens requested review from nvanbenschoten and RaduBerinde and removed request for nvanbenschoten October 17, 2023 18:33
Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @jbowens and @sumeerbhola)


vfs/mem_fs.go line 93 at r1 (raw file):

// semantics, in particular with respect to whether any of an open file's links
// may be removed. Windows semantics default to off.
func (y *MemFS) UseWindowsSemantics(windowsSemantics bool) {

[nit] I don't think we need the argument, we can just make it turn it on always.

Copy link
Collaborator Author

@jbowens jbowens left a comment

Choose a reason for hiding this comment

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

TFTR!

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @RaduBerinde and @sumeerbhola)


vfs/mem_fs.go line 93 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] I don't think we need the argument, we can just make it turn it on always.

True — I'm going to leave it to maintain an identical backport commit, but we can definitely change it on master.

@jbowens jbowens merged commit db91e5c into cockroachdb:crl-release-22.2 Oct 17, 2023
10 of 11 checks passed
@jbowens jbowens deleted the 22.2-dd12a22869 branch October 17, 2023 20:16
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.

3 participants