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

[On Hold] add LongPath support with PRI.LongPath #2584

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

0x53A
Copy link
Contributor

@0x53A 0x53A commented Aug 3, 2017

This utilizes https://github.com/peteraritchie/LongPath to re-add longpath support.

I have included it by source, because a) i had to do one little change and b) the bootstrapper is supposed to be single-file anyway.


regarding PRI.LongPath and the license:

@matthid: Could that be a problem?

Honestly, I'm not sure. The general spirit of LPGP is that modifications to the lib need to be also lgpl, but the rest of the program can be whatever. So that may be ok.

Now there are nuances with what is considered inside and outside, so it is probably not so clear cut from a legal standpoint.

@peteraritchie, what do you think? Would you be OK with us using your lib in source form in paket? The reason for using source is that the exe should be standalone.

If yes, how would you like to be credited?

Thanks!

@0x53A 0x53A changed the title add Long pat add LongPath support with PRI.LongPath Aug 3, 2017
@0x53A 0x53A changed the title add LongPath support with PRI.LongPath [WIP] add LongPath support with PRI.LongPath Aug 3, 2017
@@ -0,0 +1,165 @@
GNU LESSER GENERAL PUBLIC LICENSE
Copy link
Member

Choose a reason for hiding this comment

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

Could that be a problem?

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 think it should be ok, but let's see if the author answers.

@peteraritchie
Copy link

Sure, as long as you're following the license, use the LongPath source code directly. Seems to fall under [Combined Work](https://www.gnu.org/copyleft/lesser.html#section4. Basically, same license (unless it's commercial), make the source available, detail any modifications, and credit and/or include copyright notice if your software details its own copyright. And no expectation of support or warrantee (if you encounter a problem, I'll see what I can do, but can't guarantee anything :) )

A note on the GitHub repo page like "Long path support provided by PRI.LongPath: https://github.com/peteraritchie/longPath" is fine

@matthid
Copy link
Member

matthid commented Aug 4, 2017

@0x53A To be honest I'm still a bit worried about the answer but I'm not a lawyer.
Similar issues (for example rubychan/coderay#25) have been solved by switching to MIT. I think LGPL and GPL add additional restrictions to us and I don't want to think about legal stuff when using paket or paket source code at work. This change - because quite intrusive - makes that more difficult for me.

Maybe I'm just overly concerned here, and I clearly can see the benefit of your work. So I can compromise with this if we could enable and disable the usage of this lib by a feature flag (#if LGPL for example), because most end-users may not share my concerns (and only use the binary and not the source-code).

What do you think?

/cc @peteraritchie
/cc top contributors @forki @theimowski @agross @cloudRoutine @mrinaldi @vbfox @ctaggart @pblasucci @Stift @mexx @enricosada @smoothdeveloper

@0x53A
Copy link
Contributor Author

0x53A commented Aug 4, 2017

GPL - absolutely, this, as Ballmer put it, infects the whole source like a cancer.
LGPL - IANAL, but this mostly affects the part that is lgpl licensed itself.

From my understanding, especially section 4 and 5 of https://www.gnu.org/copyleft/lesser.html#section4:

  • We DO need to license all changes to Pri.LongPath under LGPL.

  • We DO need to place a notice somewhere, linking to the original work.

  • 4.a) from my understanding, the note in the readme is enough

  • 4.b) the lgpl license text is included with our copy of the source code of Pri.LongPath

  • 4.c) afaik we never display a copyright notice during execution, so does not apply

  • 4.d) paket is open-source, so the end-user may modify and recompile it

  • 5.a) Our version of Pri.LongPath is open-source itself, I understand that to satisfy it

  • 5.b) from my understanding, the note in the readme is enough


This whole stuff would be much easier if we just consumed it as a lib, but since paket.bootstrapper is single-file that is not possible, and I think just using ilmerge would also place some of these additional requirements on us.


Just two additional notes:

  • all requirements are to the one distributing the derived lib/application, NOT to the end-user. The (l)gpl licenses were created specially to protect the right of the end-user.
  • (IANAL, but there was a report at heise some time ago) Only the original author may sue for a (l)gpl violation. So even if we violated the letter of the lgpl, peteraritchie himself would need to sue us.

@matthid

So I can compromise with this if we could enable and disable the usage of this lib by a feature flag (#if LGPL for example).

You can disable it by just removing all the opens. The types in Pri.LongPath shadow the types in System.IO. I could also make it more explicit by wrapping every open in a #if/#def.


Closing comment: I would happily close this, if you know a better way to re-add long-path support. Unfortunately the manifest based approach broke on lower versions of windows. (Thanks Microsoft!).

@0x53A
Copy link
Contributor Author

0x53A commented Aug 4, 2017

Hm, a few tests fail, probably due to differences between Pri and System.IO.

image

Will work on these after/if the license issue has been sorted out (whatever the resolution may be)

@vbfox
Copy link
Contributor

vbfox commented Aug 10, 2017

I'm not a fan of including LGPL code and would prefer if paket did without, but I don't think there is any real legal problem (IANAL)

The only real problem doesn't really concern the paket ecosystem as it is. It would be if one day some proprietary IDE wants to include the boostrapper in their codebase as they would need to provide a way to drop-in replace this library but that's relatively easy by including the nuget version instead (A full IDE wouldn't care about the single file aspect)


I'm a lot more worried about including the full library in the boostrapper, impact on file size seem to be +85% (53K -> 98K) that's VERY drastic for a bugfix that nearly never impact it (Boostrapper doesn't create package hierarchies)

I would prefer for the bootstrapper to have just the few things it uses re-implemented to be long-path aware.


On the library code itself i'm worried that it consider "mono" to mean non-windows as it's a recipe for problems.
Both because mono run on windows (And their impl isn't long path aware AFAIK) and dotnetcore run on linux & macs...

The lib doesn't seem to be prepared for the modern multi-framework multi-platform .Net world so we would need to PR changes there to add such support.

@matthid
Copy link
Member

matthid commented Aug 18, 2017

I pretty much agree with @vbfox on all points. However, there is one addition : We could still take this AND leave the bootstrapper as-is. Which would basically remove the bootstrapper argument (tbh it is more useful in the main code anyway)

So I already said that I'm not a huge fan. Though if people say it is useful we could take it.

Note that there still is a way to do it entirely different: There was a PR which tried to set some app.config stuff to enable long-paths. This didn't work out because of support for older systems and mono? We could fix that by making the boostrapper write/patch the app.config if we detect the correct system.

@0x53A 0x53A changed the title [WIP] add LongPath support with PRI.LongPath [Shelved] add LongPath support with PRI.LongPath Aug 19, 2017
@0x53A 0x53A changed the title [Shelved] add LongPath support with PRI.LongPath [On Hold] add LongPath support with PRI.LongPath Aug 19, 2017
@0x53A
Copy link
Contributor Author

0x53A commented Aug 19, 2017

Ok thank you all for the feedback.

Note that there still is a way to do it entirely different: There was a PR which tried to set some app.config stuff to enable long-paths. This didn't work out because of support for older systems and mono? We could fix that by making the boostrapper write/patch the app.config if we detect the correct system.

This could actually work, but feels like a big hack, and may cause issues if you want to invoke paket.exe directly. Or it would need to copy itself to %temp%, and relaunch itself with the modified app.config...

Is there really no official Microsoft-supported way?

I'm still on vacation, and after that I would like to work on the other prs first (toolset 2, for example), so this will be a long time away anyway.

But unless we find a third, better way, I think I prefer longpath support via some library to copying and modifying the app.config.

@matthid
Copy link
Member

matthid commented Aug 19, 2017

This could actually work, but feels like a big hack, and may cause issues if you want to invoke paket.exe directly

Yes, you would only "get" the feature if you are using "Magic"-Mode. This would be a design decision if we go down this route.

Is there really no official Microsoft-supported way?

I don't know any besides setting the configuration. It really is a shame that the config is not ignored on older systems. Maybe there is such a way to make it work?

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.

None yet

4 participants