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

Support SPDX license identifiers (see #292) #294

Closed
wants to merge 0 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@sol
Owner

sol commented Jul 10, 2018

  • Require cabal-version: 2.2 when SPDX license identifiers are used
  • Map cabal-style licenses to SPDX license identifiers when
    cabal-version is 2.2 or higher
@quasicomputational

LGTM, one ignorable suggestion aside.

Left _ -> case cabalLicense of
Right l -> CanSPDX (Cabal.licenseToSPDX l)
Left _ -> DontTouch
where

This comment has been minimized.

@quasicomputational

quasicomputational Jul 10, 2018

Contributor

I think it might be clearer to write these cases like this:

  Right l | isRight cabalLicense -> CanSPDX l
  Right l -> MustSPDX l
  Left  _ -> case cabalLicense of
    Right l -> CanSPDX (Cabal.licenseToSPDX l)
    Left _ -> DontTouch

Then we can also get rid of the hard-coded knocnLicenses list.

This comment has been minimized.

@quasicomputational

quasicomputational Jul 10, 2018

Contributor

Oh, wait, that'll go wrong because UnknownLicense exists. Ugh, annoying.

This comment has been minimized.

@sol

sol Jul 10, 2018

Owner

Yes, exactly. Right now we don't touch unknown stuff so that Cabal will fail on typos in SPDX license identifiers. Otherwise e.g. GPL-2.0-olny would be converted to LicenseRef-GPL-2.0-olny, which I assume we don't want.

This comment has been minimized.

@sol

sol Jul 11, 2018

Owner

I added one more test to make it more clear what's happening here: 4d23731

@quasicomputational

This comment has been minimized.

Contributor

quasicomputational commented Jul 10, 2018

CI should be fixed by just bumping the stack resolver to something recent.

@sol

This comment has been minimized.

Owner

sol commented Jul 10, 2018

Anybody knows what the AppVeyor build issue is about? Apparently due to network?! @kazu-yamamoto

@tfausak

This comment has been minimized.

Collaborator

tfausak commented Jul 10, 2018

Using the LTS 12.0 resolver instead of a nightly might fix the network build problem on AppVeyor.

@quasicomputational

This comment has been minimized.

Contributor

quasicomputational commented Jul 10, 2018

Oh, I do know this one. In fact I helped fix it in Cabal HEAD! commercialhaskell/stack#3944 has all the details, but the TLDR is that setting TMP: "c:\\tmp" in the AppVeyor config fixes it.

@sol

This comment has been minimized.

Owner

sol commented Jul 10, 2018

@quasicomputational do you know if a different stackage snapshot as suggested by @tfausak will also help?

@quasicomputational

This comment has been minimized.

Contributor

quasicomputational commented Jul 10, 2018

No, I don't think it will. The problem is that the temporary directory generated by AppVeyor looks like C:\1\blah - note the backslashes, which cause problems for the configure script. Manually overriding the temporary directory works because the specific problem is a backslash followed by a number; there's a fuller fix in Cabal HEAD (swap backslashes for slashes on Windows), but this is a good workaround on AppVeyor today.

@tfausak

This comment has been minimized.

Collaborator

tfausak commented Jul 10, 2018

Ah, yup. LTS 12 doesn't fix this problem on AppVeyor: https://ci.appveyor.com/project/TaylorFausak/rattletrap/build/446#L336

@kazu-yamamoto

This comment has been minimized.

kazu-yamamoto commented Jul 10, 2018

Very strange. network 2.6 should be buildable on AppVeyor: https://ci.appveyor.com/project/eborden/network/build/453

We did not change building system for 2.6.

@sol

This comment has been minimized.

Owner

sol commented Jul 11, 2018

there's a fuller fix in Cabal HEAD (swap backslashes for slashes on Windows), but this is a good workaround on AppVeyor today.

@quasicomputational this sounds scary. I'm surprised that this wasn't included in a bug fix release yet.

@sol

This comment has been minimized.

Owner

sol commented Jul 11, 2018

Still failing on Windows, not sure what the gist of it is...

@sol sol closed this Jul 11, 2018

@sol sol deleted the dev branch Jul 11, 2018

@sol

This comment has been minimized.

Owner

sol commented Jul 11, 2018

I oppened #295 for the failing AppVeyor build.

@sol

This comment has been minimized.

Owner

sol commented Jul 11, 2018

Merged! @quasicomputational thanks for the review :)

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