Skip to content

Fix issue 17167 - handle long filepaths on Windows#7299

Merged
dlang-bot merged 4 commits intodlang:masterfrom
kaleidicassociates:fix_issue_17167
Nov 15, 2017
Merged

Fix issue 17167 - handle long filepaths on Windows#7299
dlang-bot merged 4 commits intodlang:masterfrom
kaleidicassociates:fix_issue_17167

Conversation

@atilaneves
Copy link
Contributor

This issue is made more grave by the fact that dub uses relative file paths for any package dependencies. This means that if your project is in C:\foo\bar\baz\quux, it'll try writing to C:\foo\bar\baz\quux\..\..\..\..\Users\%USERNAME%\AppData\Roaming\dub\packages\%PACKAGE_VERSION%\%PACKAGE%\.dub\%HASH%. This gets big quite quickly and goes over Windows's 260 character file path limit, meaning that dub build fails with a message saying that it can't write to a certain directory.

This PR fixes the issue by using the newer unicode-aware Win32 APIs, which don't have the limitation.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @atilaneves! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

  • smaller, focused PRs are easier to review than big ones

  • try not to mix up refactoring or style changes with bug fixes or feature enhancements

  • provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close Bugzilla Description
17167 dmd fails to write to file or create directory with more than 248 characters in the path

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️


Please consult
https://msdn.microsoft.com/en-us/library/windows/desktop/aa363855(v=vs.85).aspx
*/
Copy link
Member

Choose a reason for hiding this comment

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

Please use Ddoc style comments. In particular, this needs a Params: section, a Returns: section, and the Please consult should be References:. Ditto for the rest of the added functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

const lastError = GetLastError();

// Preserve compatibility with mkdir since the calling code expects
// errno to be set.
Copy link
Member

Choose a reason for hiding this comment

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

Don't need compatibility since there is only one caller of this private function. Please don't use errno on Windows unless forced to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

const length = MultiByteToWideChar(0 /*codepage*/, 0 /*flags*/, str, strLength, null, 0);
if (!length) return null;

auto ret = length > buf.length ? new wchar[length] : buf;
Copy link
Member

Choose a reason for hiding this comment

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

The new is a memory leak in dmd. Take a look at https://github.com/dlang/phobos/blob/master/std/internal/cstring.d#L122 where malloc is used for the temporary buffer, cleaned up with free().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that leaking was the memory strategy for dmd in order for it to be fast and that at some point the GC would be turned on. The code you linked is from Phobos, so do you mean I should create a similar struct for dmd or just to use free to clean up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed how things are done now. The leak is no longer there, at the sake of some more complexity.

@WalterBright
Copy link
Member

This problem was fixed in Phobos in std.file long ago :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants