search

arclanguage / anarki

Community-managed fork of the Arc dialect of Lisp; for commit privileges submit a pull request.
http://arclanguage.github.io
Other
1.17k stars 160 forks source link

Implement `set-c[ad]r!` properly on Racket CS. #183

Closed samth closed 3 years ago

samth commented 3 years ago

Fixes test failure https://plt.eecs.northwestern.edu/pkg-build/server/built/test-fail/anarki.txt

rocketnia commented 3 years ago

Thanks so much for the help @samth. Could you rebase this pull request? I think I fixed what was making the CI script fail.

samth commented 3 years ago

Rebased.

rocketnia commented 3 years ago

Ah, hmm, the errors this time are a bit more interesting.

For the record, here's some more detail on the 7.9 issues. (They arise in unit tests that aren't hooked into the raco test system, if you were wondering why they didn't show up for you.)

There are two tests in lib/tests/core-typing-test.arc that are overly coupled to the error message:

types.coerce.strings.string->int.coerce-positive-infinity-to-int failed: (details ex) should be "inexact->exact: no exact representation
  number: +inf.0" but instead was "exact: no exact representation for +inf.0"
[...]
types.coerce.strings.string->int.coerce-negative-infinity-to-int failed: (details ex) should be "inexact->exact: no exact representation
  number: -inf.0" but instead was "exact: no exact representation for -inf.0"

And there are two tests in arc.arc.t that are overly coupled to the hash iteration order (which I think changed in Racket 7.7 with its new HAMT-based implementation of hashes):

serialize.tables failed: (serialize (obj 1 2 3 4)) should be (tagged table ((3 4) (1 2))) but instead was (tagged table ((1 2) (3 4)))
[...]
serialize.nested-tables failed: (serialize (obj 1 (table) 2 3)) should be (tagged table ((2 3) (1 (tagged table nil)))) but instead was (tagged table ((1 (tagged table nil)) (2 3)))
rocketnia commented 3 years ago

Come to think of it, there used to be a bunch more dynamic-require calls in the codebase, and not long ago these were all replaced with require even though it meant bumping the Racket version. What do you think of bumping the version to 7.6, @akkartik, @zck, @kennethrapp, or anyone else with an active interest?

zck commented 3 years ago

I'm in favor of anything that makes Anarki easier to work on, as long as it doesn't require especially onerous work on the part of users. And "update to a Racket version released nearly two years ago" is not too difficult.

EDIT: to be explicit, I'm in favor of bumping the required Racket version to 7.6 if it makes life easier.

akkartik commented 3 years ago

In general in my own projects, I tend to weigh bumping version requirements against the benefit. If there's a one-line change that allows me to use an older version while remaining compatible with newer ones, it seems nice to be more accessible to someone on an older computer and so on.

However, my interest is not very active so I don't feel I have a vote :) I'm also sympathetic to using Anarki as a vehicle to keep up with goings-on in Racket-land.

Racket's pretty easy to reinstall, so personally having to upgrade doesn't feel like a big cost. I'd say 👍

rocketnia commented 3 years ago

Okay, could you rebase again, @samth? Thanks for your patience, and no rush.

I've updated the lowest supported version to 7.6... which I'm not sure will be quite high enough, since ffi/unsafe/vm is documented as becoming available in 7.6.0.7. (I suppose I could've obtained 7.6 locally and tried it out....)

I've also set the HEAD builds so that they're "allowed failures," which is something I've been meaning to do for a while. This may be a little counterproductive for the purpose of this ticket; it means the build might appear to pass even if the version we're trying to support here turns out to be failing. Clicking through to the Travis CI status page will reveal whether the 7.9 [cs] build is passing.

kennethrapp commented 3 years ago

I'm ok with bumping the Racket version.

rocketnia commented 3 years ago

Ah, indeed, 7.6 wasn't late enough. I've bumped up the minimum version to 7.7. This time I ran the tests locally on 7.7, and it seems to be working.

Could you rebase, @samth?

akkartik commented 3 years ago

Thank you both!

samth commented 3 years ago

Rebased again.

rocketnia commented 3 years ago

Thank you so much @samth. All the checks have passed! Gonna merge it. :)