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

[WIP] Distribution.SPDX.LicenseId generation #4913

Merged
merged 1 commit into from Dec 24, 2017

Conversation

phadej
Copy link
Collaborator

@phadej phadej commented Nov 26, 2017

Please include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

Please also shortly describe how you tested your change. Bonus points for added tests!

@23Skidoo 23Skidoo changed the title Spdx gen 2 [WIP] Spdx gen 2 Nov 27, 2017
@phadej phadej force-pushed the spdx-gen-2 branch 2 times, most recently from c316685 to 5e6c233 Compare December 14, 2017 08:42
@phadej phadej changed the title [WIP] Spdx gen 2 Distribution.SPDX.LicenseId generation Dec 14, 2017
@phadej
Copy link
Collaborator Author

phadej commented Dec 14, 2017

Let's start small and get this merged as it is now.
I'll continue on SPDX stuff (next week the latest).

@phadej phadej force-pushed the spdx-gen-2 branch 4 times, most recently from 23dcce2 to ce8a228 Compare December 15, 2017 22:29
@phadej phadej requested review from 23Skidoo and hvr December 15, 2017 22:30
@@ -353,7 +353,7 @@ isAsciiAlpha c = ('a' <= c && c <= 'z')
-- False
--
isAsciiAlphaNum :: Char -> Bool
isAsciiAlphaNum c = isAscii c || isDigit c
isAsciiAlphaNum c = isAscii c && isAlphaNum c
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this weren't used anywhere before :S

@phadej
Copy link
Collaborator Author

phadej commented Dec 15, 2017

This PR introduces SPDX modules. They aren't yet used in parsing .cabal files (need to get #4953 merged first)

@23Skidoo 23Skidoo changed the title Distribution.SPDX.LicenseId generation [WIP] Distribution.SPDX.LicenseId generation Dec 19, 2017
Copy link
Member

@23Skidoo 23Skidoo left a comment

Choose a reason for hiding this comment

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

LGTM modulo minor comments.

@@ -1 +1,2 @@
packages: cabal-dev-scripts
other-packages:
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to not include */*.cabal which causes it resolve to weird things.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I meant it should be optional-packages, will fix that.

travis-meta.sh Outdated
# Regenerate files
timed make gen-extra-source-files
timed make spdx
Copy link
Member

Choose a reason for hiding this comment

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

👍

Would be nice to fix make lexer as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried, it failed on travis (there were whitespace difference, no idea why)

Copy link
Member

Choose a reason for hiding this comment

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

Different version of alex or some of its dependencies maybe? Though I have 3.1.7 from @hvr's PPA installed and the diff after make lexer is empty.

-- Licenses
-------------------------------------------------------------------------------

data License = License
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be LicenseException? A bit confusing that this one and the one in GenSPDX.hs have the same name, but are slightly different. Maybe move these types to a common module as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It should. I'll add a TODO note. I'm not keen on refactoring that too much now, as it might need bigger changes (hopefully not). It can be done after the release, as it's not part of delivery

Distribution.SDPX.LicenseId and Distribution.SDPX.LicenceExceptionId are
generated.
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.

None yet

2 participants