Skip to content

Only use Path.Combine on netcore#4

Merged
Olof-Lagerkvist merged 2 commits into
LTRData:LTRData.DiscUtils-initialfrom
gfs:gfs/PathCombineIssue
Jan 26, 2024
Merged

Only use Path.Combine on netcore#4
Olof-Lagerkvist merged 2 commits into
LTRData:LTRData.DiscUtils-initialfrom
gfs:gfs/PathCombineIssue

Conversation

@gfs

@gfs gfs commented Jan 26, 2024

Copy link
Copy Markdown

Path.Combine has more restrictive (windows specific) behavior on Net Framework which can present problems with extracting files from formats like .dmg.

For example, this attached dmg (created from a folder with two text files using hdiutil on Mac OS, GitHub doesn't allow dmgs to be directly attached so I have zipped it) causes an Argument Exception on net4.8 but not net6 - net8 with the current code, this change resolves that argument exception allowing the dmg to also be extracted by net4.8 based programs using DiscUtils.

HfsSampleUDCO.zip

See also: microsoft/RecursiveExtractor#145

Path.Combine has more restrictive (windows specific) behavior on Net Framework which can present problems with extracting files from formats like .dmg.
@Olof-Lagerkvist

Copy link
Copy Markdown
Member

That's a good point. Although, it will still break if someone uses the .NET Standard 2.0 version in a .NET Framework project. That is because Path.Combine does not really have a "standard" behavior for this specified in .NET Standard 2.0.

The more I think of this, we should probably go even further and only use Path.Combine in .NET/.NET Core specifically.

@gfs

gfs commented Jan 26, 2024

Copy link
Copy Markdown
Author

I wasn't sure about the netstandard behavior but I think you're correct. Probably best to just have it on net core. I can push another commit with that.

@gfs gfs changed the title Only use Path.Combine on netstandard or netcore Only use Path.Combine netcore Jan 26, 2024
@gfs gfs changed the title Only use Path.Combine netcore Only use Path.Combine on netcore Jan 26, 2024
@Olof-Lagerkvist Olof-Lagerkvist merged commit 474f44a into LTRData:LTRData.DiscUtils-initial Jan 26, 2024
@Olof-Lagerkvist

Copy link
Copy Markdown
Member

Merged. Thanks a lot for your contribution!

@gfs

gfs commented Jan 26, 2024

Copy link
Copy Markdown
Author

🥳 No problem. Thank you for picking up the torch on discutils!

@Erik-White

Copy link
Copy Markdown

@LTRData Would you mind bumping the version and publishing a new nuget package please?

@Olof-Lagerkvist

Copy link
Copy Markdown
Member

@LTRData Would you mind bumping the version and publishing a new nuget package please?

Sure! That will be done either later today or tomorrow. I might have another change as well for a new version and if it seems good, I would like to include it too.

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