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

FileSystemEntry.Attributes property is not correct on Unix #37301

Closed
iSazonov opened this issue Jun 2, 2020 · 17 comments · Fixed by #40641 or #52235
Closed

FileSystemEntry.Attributes property is not correct on Unix #37301

iSazonov opened this issue Jun 2, 2020 · 17 comments · Fixed by #40641 or #52235
Assignees
Labels
area-System.IO Cost:S Work that requires one engineer up to 1 week Priority:3 Work that is nice to have tenet-performance Performance related issue
Milestone

Comments

@iSazonov
Copy link
Contributor

iSazonov commented Jun 2, 2020

Description

Comes from PowerShell/PowerShell#12834 where custom file enumerator is replaced with .Net FileSystemEnumerator.

Expectation is that:

  1. FileSystemEntry.Attributes property works the same as FileSystemInfo.Attributes property for Hidden (and perhaps ReadOnly) flags.
  2. For Extended Unix flags and Windows Reparse points we need new property FileSystemInfo.ExtendedAttributes.

It turns out the first has a simplified implementation

entry._status = default;
FileStatus.Initialize(ref entry._status, isDirectory);
FileAttributes attributes = default;
if (isSymlink)
attributes |= FileAttributes.ReparsePoint;
if (isDirectory)
attributes |= FileAttributes.Directory;
if (directoryEntry.Name[0] == '.')
attributes |= FileAttributes.Hidden;
if (attributes == default)
attributes = FileAttributes.Normal;
entry._initialAttributes = attributes;
return attributes;

than the second

FileAttributes attributes = default;
if (IsReadOnly(path))
attributes |= FileAttributes.ReadOnly;
if ((_fileStatus.Mode & Interop.Sys.FileTypes.S_IFMT) == Interop.Sys.FileTypes.S_IFLNK)
attributes |= FileAttributes.ReparsePoint;
if (_isDirectory)
attributes |= FileAttributes.Directory;
// If the filename starts with a period or has UF_HIDDEN flag set, it's hidden.
if (fileName.Length > 0 && (fileName[0] == '.' || (_fileStatus.UserFlags & (uint)Interop.Sys.UserFlags.UF_HIDDEN) == (uint)Interop.Sys.UserFlags.UF_HIDDEN))
attributes |= FileAttributes.Hidden;
return attributes != default ? attributes : FileAttributes.Normal;

Also CHFLAGS works only on BSD-like systems (MacOS, FreeBSD) but not on Linux. So expectation is that assigning Hidden flag on Linux throws PlatformNotSupportedException - it was very amazing to discover that some PowerShell tests are passed on Linux but really Hidden flag does not work.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.IO untriaged New issue has not been triaged by the area owner labels Jun 2, 2020
@danmoseley
Copy link
Member

Was this just an oversight in dotnet/corefx#34560

@iSazonov
Copy link
Contributor Author

iSazonov commented Jun 4, 2020

@davidkaya @danmosemsft @JeremyKuhne Could you please look the issue? I'd be happy to get the fix for PowerShell.

@JeremyKuhne
Copy link
Member

@danmosemsft Yes, this looks like an oversight. We should add a comment in both places pointing out the connection if we can't share the code.

@iSazonov I'm no longer on the libraries team, but this should clearly be consistent between the two. @carlossanlop will have to decide the best course of action on throwing. If we can be confident on checking that it isn't supported than throwing seems reasonable to me at first glance.

@carlossanlop carlossanlop removed the untriaged New issue has not been triaged by the area owner label Jun 5, 2020
@carlossanlop carlossanlop added this to the Future milestone Jun 5, 2020
@iSazonov
Copy link
Contributor Author

If the fix is "to throw" the issue should be fixed in 5.0 milestone - later it will be a breaking change.

@carlossanlop carlossanlop modified the milestones: Future, 5.0.0 Jun 25, 2020
@carlossanlop
Copy link
Member

@iSazonov I marked it for 5.0. Would you have a chance to work on this in the next few days? If not, @jozkee or I can take over when we get some bandwidth.

@iSazonov
Copy link
Contributor Author

@carlossanlop I have not possibility to develop on Unix :-(

@danmoseley
Copy link
Member

@iSazonov even with WSL 2 ? 😃

@danmoseley danmoseley added the help wanted [up-for-grabs] Good issue for external contributors label Jun 25, 2020
@iSazonov
Copy link
Contributor Author

@danmosemsft I would like, but my laptop is against 😃

@carlossanlop
Copy link
Member

@iSazonov can you take a look at the PR, please?

@iSazonov
Copy link
Contributor Author

iSazonov commented Sep 4, 2020

@carlossanlop Can we re-open the issue?

@billwert
Copy link
Member

@daxian-dbw hit this when trying to move Powershell to RC1. Here's the test we should ensure works well when we address this for 6.0:

Measure-Command { TabExpansion2 -inputScript "Invoke-Web" -cursorColumn "Invoke-Web".Length }

In WSL2 if that takes more than a couple hundred milliseconds, there's a problem. (In .NET 5 RC1 it is ~2.5 seconds due to all the stating.)

@jeffhandley jeffhandley self-assigned this Jan 14, 2021
@jeffhandley jeffhandley added the Priority:3 Work that is nice to have label Jan 14, 2021
@carlossanlop carlossanlop added the Cost:S Work that requires one engineer up to 1 week label Jan 14, 2021
@iSazonov
Copy link
Contributor Author

Priority:3

I wonder how you weight this:

  1. There is two code paths which contradict each other
  2. There is a clear performance issue
  3. Finally, it's just bad design, even wrong

@jeffhandley
Copy link
Member

@iSazonov Thanks for the comment and feedback. The priority is based on our overall themes and the relative priority of fixing this issue compared to completing other items that the team is committing to for 6.0.0. Even with Priority:3, our project plan includes time to work on this issue during the 6.0.0 release cycle.

@iSazonov
Copy link
Contributor Author

@jeffhandley Thanks for clarify! I am trying to speed up PowerShell Core file operation and I hope we get this in the PowerShell milestone. So I have many concerns about .Net file API design and performance. Feel free to ping me when will your team start this work.

@jeffhandley
Copy link
Member

@iSazonov What timing would enable you to utilize this fix in the PowerShell milestone you're targeting?

@iSazonov
Copy link
Contributor Author

@jeffhandley MSFT PowerShell team said they want to stop big changes before summer and then work only on bugs and stability.

@danmoseley danmoseley added the tenet-performance Performance related issue label Jan 20, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 22, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 31, 2021
@iSazonov
Copy link
Contributor Author

iSazonov commented Apr 21, 2021

I added in the OP that is right design should be - "For Extended Unix flags and Windows Reparse points we need new property FileSystemInfo.ExtendedAttributes."

Update: It should be File Flags. Extended Attributes is another conception.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 4, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 8, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.