test: Run symlink regression tests on Windows #24381

MarcoFalke commented at 11:16 AM on February 18, 2022: member

Seems odd to add tests, but not run them on the platform that needs them most.

vasild commented at 11:21 AM on February 18, 2022: member

Hmm, symlinks don't exist on Windows (or more precisely on Windows-supported filesystems like FAT16, FAT32, NTFS, whatnot?).

ghost commented at 12:11 PM on February 18, 2022: none

Hmm, symlinks don't exist on Windows (or more precisely on Windows-supported filesystems like FAT16, FAT32, NTFS, whatnot?).

The Windows’ NTFS file system has supported symlinks since Windows Vista.

https://blogs.windows.com/windowsdeveloper/2016/12/02/symlinks-windows-10/

MarcoFalke commented at 12:16 PM on February 18, 2022: member

Symlinks exist on Windows, otherwise the test wouldn't pass (https://github.com/bitcoin/bitcoin/pull/24338/files#r808801652).

Though, it looks like symlinks don't exist on mingw: https://gcc.gnu.org/git/?p=gcc.git;a=blobdiff;f=libstdc%2B%2B-v3/testsuite/27_io/filesystem/operations/create_directories.cc;h=304c1453afe650b1e5850114df4d86772d6eb4f4;hp=393d6a5530925bd1305eaf460c107954cb0b1ab9;hb=124eaa50e0a34f5f89572c1aa812c50979da58fc;hpb=e07d30fdcaec4906e0dcb948fc4748bf74c15c05

MarcoFalke force-pushed on Feb 18, 2022

ryanofsky approved

ryanofsky commented at 12:39 PM on February 18, 2022: member

Code review ACK fa1919014c23ef8f80e7893e1a4cee9bfa375eb9

It's an anti-pattern to skip tests on windows, and this has caused bugs recently. We should almost never be skipping tests on windows. If something is expected to fail on windows, the test should run and then check for the expected failure.

vasild commented at 12:52 PM on February 18, 2022: member

Does the test pass only if the underlying filesystem is NTFS? Would it fail on a FAT filesystem?

DrahtBot added the label Tests on Feb 18, 2022

MarcoFalke force-pushed on Feb 18, 2022

MarcoFalke commented at 2:18 PM on February 18, 2022: member

Looks like the new test uncovered a new libc++ bug in versions 11 and lower? Removed new test for now. I'll debug this as a follow-up.

Should be trivial to re-ACK.

MarcoFalke force-pushed on Feb 18, 2022

test: Run symlink regression tests on Windows fad7ddf9e3

MarcoFalke force-pushed on Feb 18, 2022

ryanofsky approved

ryanofsky commented at 2:55 PM on February 18, 2022: member

Code review ACK fad7ddf9e3710405d727f61d8200d5efed1e705b, just removing new test. Would be nice if the test could be added later, of course.

Does the test pass only if the underlying filesystem is NTFS? Would it fail on a FAT filesystem?

Probably trying to create symlinks on a fat drive would fail whether the test is running on windows or linux. Maybe it'd be useful to have CI build that runs tests on a FAT filesystem (if this is something we support). I'd be surprised if the tests already pass already and there wouldn't be other bugs to fix.

MarcoFalke commented at 3:42 PM on February 18, 2022: member

The multiwallet test already uses symlinks unconditionally, so this pull isn't changing where test can be run. Is FAT still a thing in 2022? If yes, it might be better to start a new issue/thread to discuss testing on it.

luke-jr commented at 1:14 AM on February 19, 2022: member

Aside from FAT, Android also prohibits symlinks on the /sdcard filesystem

:/sdcard # ln -s a b
ln: cannot create symbolic link from 'a' to 'b': Operation not permitted

unknown commented at 1:37 AM on February 19, 2022: none

Concept ACK

Aside from FAT, Android also prohibits symlinks on the /sdcard filesystem :/sdcard # ln -s a b @luke-jr this works for me because file system is ext4. Symlinks work on Android, it's just FAT file system that does not support it.

in src/test/fs_tests.cpp:155 in fad7ddf9e3

 151 | @@ -152,7 +152,7 @@ BOOST_AUTO_TEST_CASE(rename)
 152 |      fs::remove(path2);
 153 |  }
 154 |  
 155 | -#ifndef WIN32
 156 | +#ifndef __MINGW64__ // no symlinks on mingw

hebasto commented at 8:31 AM on February 19, 2022:

Also see https://github.com/bitcoin/bitcoin/pull/24338/files#r808540886

hebasto commented at 8:32 AM on February 19, 2022: member

Concept ACK.

luke-jr commented at 11:46 PM on February 22, 2022: member

@luke-jr this works for me because file system is ext4. Symlinks work on Android, it's just FAT file system that does not support it.

On Android, /sdcard is "sdcardfs", which doesn't support symlinks.

in test/functional/feature_dirsymlinks.py:24 in fad7ddf9e3

  25 | +class SymlinkTest(BitcoinTestFramework):
  26 |      def set_test_params(self):
  27 |          self.num_nodes = 1
  28 |  
  29 |      def run_test(self):
  30 | +        dir_new_blocks = self.nodes[0].chain_path / "new_blocks"

laanwj commented at 12:58 PM on February 23, 2022:

Any specific reason to assign these to variables? They're only used once.

MarcoFalke commented at 1:15 PM on February 23, 2022:

Sorry, no reason. I needed this for the follow-up test. Though, the test uncovered another bug (https://github.com/bitcoin/bitcoin/pull/24381#issuecomment-1044598093), so I removed it.

Happy to revert the variables as well. Lmk?

laanwj commented at 1:22 PM on February 23, 2022:

No, if you need it for a follow-up test it's fine.

in test/functional/feature_dirsymlinks.py:40 in fad7ddf9e3

  46 |  
  47 |          self.start_node(0)
  48 |  
  49 |  
  50 | -if __name__ == '__main__':
  51 | +if __name__ == "__main__":

laanwj commented at 1:00 PM on February 23, 2022:

Why change the quote style here? Looking at the current .py files the syntax here varies wildly. I mean, I'm not against making it consistent, but changing just this file seems pointless.

MarcoFalke commented at 1:16 PM on February 23, 2022:

No reason. I think my editor/formatter did this. Happy to revert.

laanwj commented at 1:22 PM on February 23, 2022:

No strong opinion on reverting. I was just wondering.

laanwj commented at 1:07 PM on February 23, 2022: member

It's an anti-pattern to skip tests on windows, and this has caused bugs recently.

Look, I had no idea symlinks were a reliable feature on Windows (they were not for a long time) and am unable to test anything on Windows myself. This test was added to test a workaround for a bug in std::fs on gcc/glibc. Disabling it for windows seemed to make sense. I'm fine with enabling it, but I don't think I committed some anti-patttern sin by doing it that way.

Edit: also, if there are no symlinks in mingw, the functional test will fail with mingw-built (like the distribution binary) bitcoind, right?

Edit.2: I agree that running the test on obscure or old filesystems like VFAT or some android partition isn't relevant, it makes sense to assume a fully-featured FS (as the operating system allows for) for the tests.

MarcoFalke commented at 1:19 PM on February 23, 2022: member

I'm fine with enabling it, but I don't think I committed some anti-patttern sin by doing it that way.

I think it is fine to initially add a test for a specific operating system or exclude a specific operating system. A test is better than no test.

ryanofsky commented at 2:26 PM on February 23, 2022: member

Nope, laanwj you have sinned and I pronounce you guilty of crimes against test coverage! But seriously, if you want feature X to work on all platforms and you disable tests for feature X on certain platforms, it's just a red flag. You don't have to go to jail, or devote yourself to fixing the test when there are more important things to do, and my comment was motivated more by #24251 than the create_directories thing. I just think skipping tests without a very specific and definitive reason is something people should be suspicious of.

laanwj commented at 2:48 PM on February 23, 2022: member

LOL :smile:

Code review ACK fad7ddf9e3710405d727f61d8200d5efed1e705b

laanwj merged this on Feb 23, 2022

laanwj closed this on Feb 23, 2022

MarcoFalke deleted the branch on Feb 23, 2022

sidhujag referenced this in commit a02f10718d on Feb 23, 2022

MarcoFalke commented at 4:25 PM on February 23, 2022: member

Would be nice if the test could be added later, of course.

Done in https://github.com/bitcoin/bitcoin/pull/24432

DrahtBot locked this on Feb 23, 2023