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

Store file id in an enum #494

Merged
merged 2 commits into from
Aug 8, 2023
Merged

Store file id in an enum #494

merged 2 commits into from
Aug 8, 2023

Conversation

dfaust
Copy link
Member

@dfaust dfaust commented Jun 15, 2023

This turns the FileId struct into an enum with three variants:

  • Inode
  • LowRes
  • HighRes

The variants are chosen depending on the operating system and on windows depending on the windows API function used.

My idea behind these changes was to make file-id more "stable" when using the serialization feature and also to make is more "correct".
To be honest, I'm not sure if it is actually worth it.
Then again the complexity is only marginally higher and if memory footprint is a concern, users are free to copy the values into their own data structure.
What do you guys think?

  • CHANGE: switch from winapi to windows-sys
  • CHANGE: turn FileId struct into an enum
  • FEATURE: support for high resolution file ids on Windows using GetFileInformationByHandleEx

@0xpr03
Copy link
Member

0xpr03 commented Jun 15, 2023

I'm honestly not convinced by the tradeoff. It's a lot of additional code to allow for windows XP support (which is not even Tier2 and kind of deprecated). I also had trouble finding much information about what exactly the difference between highres / lowres are. But I may be missing something.

Note that the BSD build is currently failing.

(I'll fast-track #495 before this, as I'm releasing the new requested debouncer version.)

@0xpr03
Copy link
Member

0xpr03 commented Jun 16, 2023

Then again, notify is about tackling OS complexity and getting this right once in file-id may be worth it.

@dfaust
Copy link
Member Author

dfaust commented Jun 16, 2023

... getting this right once in file-id may be worth it.

That was my idea.
Also, if we don't go for an enum, what data types to we chose?

Right now it's 64 + 64 bits (Linux and MacOS), or should it be 64 + 128 bits to accommodate the new Windows types?

Just to clarify, the LowRes variant (32 + 64 bits) gets constructed by the old GetFileInformationByHandle function, while the HighRes variant (64 + 128 bits) gets constructed by the new GetFileInformationByHandleEx function.

I don't expect there to be any additions to these types any time soon, but an enum would make that possible.

The alternative is to stick to the 64 + 64 bits struct and use the 64 lower bits of the 128 bits type for windows. This would be the same behavior as with the winapi crate.

@0xpr03
Copy link
Member

0xpr03 commented Jun 20, 2023

Then let's do this. The enum variant should be marked #[non_exhaustive] if possible (and probably abstract over it, in a way the user doesn't actually has to touch the enum variants).

@dfaust
Copy link
Member Author

dfaust commented Jun 21, 2023

Then let's do this. The enum variant should be marked #[non_exhaustive] if possible (and probably abstract over it, in a way the user doesn't actually has to touch the enum variants).

As far as I'm concerned, the user should mostly just compare and maybe serialize/deserialize ids.
I don't think the enum should be marked as non_exhaustive because it is very unlikely to change.
And if it does, then it will be a breaking change.

@0xpr03
Copy link
Member

0xpr03 commented Jul 13, 2023

Ping: this will need a rebase and a fix for BSD

@dfaust
Copy link
Member Author

dfaust commented Jul 16, 2023

@0xpr03 do you have an idea, why the BSD build is failing?

@0xpr03
Copy link
Member

0xpr03 commented Jul 16, 2023

Hm, I'd try pushing it again, the linking error makes no sense to me. If anything this should've been broken already when introducing the file-id crate initially. (And it's breaking inside rusts std.)

Sadly github actions won't let me re-run this one manually.

@dfaust dfaust force-pushed the file-id-enum branch 2 times, most recently from 915c55c to bf4b490 Compare July 17, 2023 16:17
@0xpr03
Copy link
Member

0xpr03 commented Jul 17, 2023

Something something linking error on freeBSD for file-id with ld: cannot find -lexecinfo: No such file or directory

Which is apparently some kind of GNU extension that should be existing in another way on BSD - and I can't say why it's failing now..

@0xpr03 0xpr03 mentioned this pull request Jul 18, 2023
@0xpr03
Copy link
Member

0xpr03 commented Jul 18, 2023

So what we need is a sysroot for freeBSD, otherwise the binary ends up failing to link*. Basically move all the freeBSD stuff into a container and build it inside there, as ghactions has no native bsd support.

* you just need to compile a hello world binary, unrelated to the code

@dfaust
Copy link
Member Author

dfaust commented Aug 8, 2023

👍 Good job, thanks!

@0xpr03 0xpr03 merged commit 8280160 into notify-rs:main Aug 8, 2023
13 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.

2 participants