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

protolude 0.1.6: fix ambiguous occurrences #2225

Merged

Conversation

ilovezfs
Copy link
Contributor

@ilovezfs ilovezfs commented Jul 11, 2016

Prevent PureScript build failure with protolude 0.1.6 due to
ambiguous occurrence errors arising from the following conflicts:

Protolude.fromStrict vs. Data.ByteString.Lazy.fromStrict
Protolude.decodeUtf8 vs. Data.Text.Lazy.Encoding.decodeUtf8
Protolude.encodeUtf8 vs. Data.Text.Lazy.Encoding.encodeUtf8

In order to preserve the ability to use "fromStrict" without qualifying
it (e.g., Z.fromStrict), this fixes the ambiguous occurrences in a way
that is backwards incompatible with protolude 0.1.5. In particular,
using fromStrict without qualification requires hiding it, but since
protolude 0.1.5 doesn't actually export fromStrict, importing 0.1.5
hiding fromStrict will trigger a dodgy imports warning.

Bumping the protolude dependency to >= 0.1.6 requires bumps to
PureScript's designated Stack LTS and Nightly resolvers. Switching to
lts-6.7 and nightly-2016-07-19 requires a few unrelated fixes:

System.FilePath: remove redundant imports (-Wunused-imports)
parseURL: replace with non-deprecated parseRequest (-Wdeprecations)
fromJust: import from Data.Maybe instead of Unsafe (prevents error)

Also set protolude >=0.1.6 and http-client >= 0.4.30 (for parseRequest)
in purescript.cabal, and update the CONTRIBUTORS file.

@paf31
Copy link
Contributor

paf31 commented Jul 11, 2016

Is this causing a build to break?

Also, could you please update CONTRIBUTORS.md?

@ilovezfs
Copy link
Contributor Author

Yes, GHC 8.0.1

src/Language/PureScript/Ide/Pursuit.hs:56:32: error:
    Ambiguous occurrence ‘fromStrict’
    It could refer to either ‘Protolude.fromStrict’,
                             imported from ‘Protolude’ at src/Language/PureScript/Ide/Pursuit.hs:22:1-26
                             (and originally defined in ‘Data.Text.Lazy’)
                          or ‘Data.ByteString.Lazy.fromStrict’,
                             imported from ‘Data.ByteString.Lazy’ at src/Language/PureScript/Ide/Pursuit.hs:26:49-58

src/Language/PureScript/Ide/Pursuit.hs:68:30: error:
    Ambiguous occurrence ‘fromStrict’
    It could refer to either ‘Protolude.fromStrict’,
                             imported from ‘Protolude’ at src/Language/PureScript/Ide/Pursuit.hs:22:1-26
                             (and originally defined in ‘Data.Text.Lazy’)
                          or ‘Data.ByteString.Lazy.fromStrict’,
                             imported from ‘Data.ByteString.Lazy’ at src/Language/PureScript/Ide/Pursuit.hs:26:49-58

etc.

@ilovezfs
Copy link
Contributor Author

I'll update contributors in a bit, since that will change the checksum of the PR patch link in Homebrew/homebrew-core#2905

@paf31
Copy link
Contributor

paf31 commented Jul 11, 2016

since that will change the checksum of the PR patch link

I can merge it as two separate commits.

@ilovezfs ilovezfs force-pushed the protolude-fix-ambiguous-occurences branch from ab8b376 to 27ae3b9 Compare July 12, 2016 05:13
@ilovezfs
Copy link
Contributor Author

@paf31 PR refreshed.

@kritzcreek
Copy link
Member

Sorry, looks like this happened because of protolude 0.1.5 -> 0.1.6. Should've maybe been more conservative with the bounds...

@ilovezfs
Copy link
Contributor Author

@kritzcreek Yes, that's why. Note the 🍏 for Travis now with nightly-2016-07-11 and lts-6.7

@kritzcreek
Copy link
Member

kritzcreek commented Jul 12, 2016

Ahh so CI didn't catch it, because you compile PS with a newer snapshot than the one our CI used. Allright I get it.

EDIT: Does that mean we need a protolude > 0.1.5 bound in the cabal file now?

@ilovezfs
Copy link
Contributor Author

No, actually we compile with straight up cabal, no Stack involved. But when I realized that the problem was 0.1.5, I figured I'd try bumping your LTS and Nightly for Stack, which led to some warnings failures and a hard error for fromJust. The bounds in purescript.cabal should also be bumped a bit too, I suppose.

@ilovezfs
Copy link
Contributor Author

EDIT: Does that mean we need a protolude > 0.1.5 bound in the cabal file now?

Yup.

@ilovezfs ilovezfs force-pushed the protolude-fix-ambiguous-occurences branch from 4df9cac to 839f595 Compare July 12, 2016 17:11
@ilovezfs ilovezfs changed the title protolude: fix ambiguous occurrences protolude 0.1.6: fix ambiguous occurrences Jul 12, 2016
@ilovezfs
Copy link
Contributor Author

@paf31 @kritzcreek PR refreshed.

@kritzcreek
Copy link
Member

👍 thanks!

@ilovezfs
Copy link
Contributor Author

Did something change to prevent the cc-osx-lts-sdist failure on Travis?

@kritzcreek
Copy link
Member

@ilovezfs the osx instances are a bit flaky. What a great commit message! :shipit:

@ilovezfs
Copy link
Contributor Author

What a great commit message!

Thanks :)

@phadej
Copy link
Contributor

phadej commented Jul 19, 2016

parseURL to parseRequest requires adding http-client >= 0.4.30 which defines it in the first place. Otherwise if older http-client is picked, the build will fail as well. (http-pipes re-exports http-client's Network.HTTP.Client)

Prevent PureScript build failure with protolude 0.1.6 due to
ambiguous occurrence errors arising from the following conflicts:

  Protolude.fromStrict vs. Data.ByteString.Lazy.fromStrict
  Protolude.decodeUtf8 vs. Data.Text.Lazy.Encoding.decodeUtf8
  Protolude.encodeUtf8 vs. Data.Text.Lazy.Encoding.encodeUtf8

In order to preserve the ability to use "fromStrict" without qualifying
it (e.g., Z.fromStrict), this fixes the ambiguous occurrences in a way
that is backwards incompatible with protolude 0.1.5. In particular,
using fromStrict without qualification requires hiding it, but since
protolude 0.1.5 doesn't actually export fromStrict, importing 0.1.5
hiding fromStrict will trigger a dodgy imports warning.

Bumping the protolude dependency to >= 0.1.6 requires bumps to
PureScript's designated Stack LTS and Nightly resolvers. Switching to
lts-6.7 and nightly-2016-07-19 requires a few unrelated fixes:

System.FilePath: remove redundant imports (-Wunused-imports)
parseURL: replace with non-deprecated parseRequest (-Wdeprecations)
fromJust: import from Data.Maybe instead of Unsafe (prevents error)

Also set protolude >=0.1.6 and http-client >= 0.4.30 (for parseRequest)
in purescript.cabal, and update the CONTRIBUTORS file.
@ilovezfs ilovezfs force-pushed the protolude-fix-ambiguous-occurences branch from 3ac08d7 to b2e4800 Compare July 19, 2016 17:24
@ilovezfs
Copy link
Contributor Author

@phadej Thanks for catching that. It should be fixed now.

@paf31
Copy link
Contributor

paf31 commented Jul 26, 2016

Does this add any new dependencies? We might need to rerun the license generator.

@ilovezfs
Copy link
Contributor Author

None I know of, since http-client is already in the license file.

@paf31 paf31 merged commit 15c466f into purescript:master Jul 26, 2016
@paf31
Copy link
Contributor

paf31 commented Jul 26, 2016

Thanks!

@ilovezfs
Copy link
Contributor Author

Cool, thanks @paf31!

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

4 participants