byteverse / haskell-ip

IP Address implementation
Other
20 stars 19 forks source link

Bump `text` Upper Bounds #85

Closed prikhi closed 2 years ago

prikhi commented 2 years ago

text v2.0 has switched some argument orders & now uses a Word8 array internally instead of Word16.


Hi, this is my attempt at bumping the version of text. I haven't really worked with Data.Text.Array before so would appreciate a review based on the copyI changes: https://hackage.haskell.org/package/text-2.0.1/changelog

Tested w/ GHC 9.2, both test suites passed:

$ cabal-3.8.1.0 new-test --disable-documentation -w ghc-9.2.3  
Build profile: -w ghc-9.2.3 -O1
In order, the following will be built (use -v for more details):
 - ip-1.7.5 (test:spec) (first run)
 - ip-1.7.5 (test:test) (first run)
Preprocessing test suite 'spec' for ip-1.7.5..
Preprocessing test suite 'test' for ip-1.7.5..
Building test suite 'spec' for ip-1.7.5..
Building test suite 'test' for ip-1.7.5..
Running 1 test suites...
Test suite spec: RUNNING...
Test suite spec: PASS
Test suite logged to:
/home/prikhi/code/github/haskell-ip/dist-newstyle/build/x86_64-linux/ghc-9.2.3/ip-1.7.5/t/spec/test/ip-1.7.5-spec.log
1 of 1 test suites (1 of 1 test cases) passed.
Running 1 test suites...
Test suite test: RUNNING...
Test suite test: PASS
Test suite logged to:
/home/prikhi/code/github/haskell-ip/dist-newstyle/build/x86_64-linux/ghc-9.2.3/ip-1.7.5/t/test/test/ip-1.7.5-test.log
1 of 1 test suites (1 of 1 test cases) passed.
andrewthad commented 2 years ago

It blows my mind that this was all that had to be done to support text-2. I think you got everything right. Can you add backwards-compat stuff to continue to support text-1 as well. Probably by adding a module with this:

#ifdef text(2,0,0) // I can't remember how this is actually formatted
type Codepoint = Word8
#else
type Codepoint = Word16
#endif

and then probably just CPP around copyI to deal with the arguments being reordered. If you're not able to do this, I can add it some time.

prikhi commented 2 years ago

I might have time this weekend to poke at some CPP, should be straightforward.

prikhi commented 2 years ago

hmm :thinking: I threw the Codepoint type in Data.Text.Builder.Common.Compat but didn't want to export it for the tests so I threw the same code from Compat into the IPv4Text1.hs test file:

#if MIN_VERSION_text(2, 0, 0)
type Codepoint = Word8
#else
type Codepoint = Word16
#endif

but weirdly that causes a compile failure with the literal bytestring below:

$ cabal-3.8.1.0 new-test --constraint 'text<2.0' -w ghc-9.2.2
[5 of 6] Compiling IPv4Text1        ( test/IPv4Text1.hs, /home/prikhi/Projects/git/haskell-ip/dist-newstyle/build/x86_64-linux/ghc-9.2.2/ip-1.7.5/t/test/build/test/test-tmp/IPv4Text1.dyn_o )

test/IPv4Text1.hs:79:53: error:
    numeric escape sequence out of range at character '2'
   |
79 |   "0001020304050607080910111213141516171819\
   |

It works fine if I move the CPP code to a separate module in tests/ but not sure if you'd just prefer I export the Compat module from the library & if you have a preferred module name/hierarchy for it.

Either way, I've got a version going that passes tests for both --constraint 'text>=2.0' & --constraint 'text<2.0'

andrewthad commented 2 years ago

The compile error is from a bad interaction between the C preprocessor and Haskell string literals with the continue-on-next-line backslash at the end. Try this:

"0001020304050607080910111213141516171819\ -- cpp workaround

If it doesn't take that, then do the separate module, but list it in other-modules and not exposed-modules so that it doesn't become part of the API.

That's good news that it passes in both cases. I've been avoiding the transition from text-1 to text-2 until other people started taking the plunge, and it's good to see that that seems to be happening.

prikhi commented 2 years ago

Fixed! Compiles & passes tests w/ 2.0 & pre-2.0 versions of text. I reverted the lower bounds.

Space with a comment didn't work, only a trailing space did but it produces a GHC warning. Apparently double blackslashes does work so I went w/ that & left a comment about why.

If it doesn't take that, then do the separate module, but list it in other-modules and not exposed-modules so that it doesn't become part of the API.

That was my first attempt, but cabal complains that our test is importing a hidden module.

andrewthad commented 2 years ago

I've run the tests locally and confirmed that they pass. Looks good. Merging. And thanks for doing this! At some point 6 months down the road, I will benefit from this.

prikhi commented 2 years ago

No problem! We're not using text 2.0 yet either, just tryna get ahead of future breakages :joy: