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

CI:ECL: load with :verbose and :print options writes text of file unsafely #77

Closed
wobh opened this issue Sep 30, 2015 · 5 comments
Closed
Assignees
Labels

Comments

@wobh
Copy link
Contributor

wobh commented Sep 30, 2015

For the new prime-factors example I implemented a factorization wheel with a circular list. This became an issue with the ECL tests because ECL's load with :verbose and :print set to true (or possibly just one of those), like we use by default, prints the file to the screen, but does so with *print-circle* left at nil. If the file contained a circular list load would never finish.

This could be seen as a bug in ECL but I would like to come up with a work-around. It might be enough to set *print-circle* to t in xlisp-test. Before I download ECL and start tinkering with it, what do you think about this?

Failing job: https://travis-ci.org/wobh/xlisp/jobs/82872525

@verdammelt
Copy link
Member

That sounds like an interesting implementation and I'd love to see it. (It does sound like it might go against my "don't be clever in example.lisp" pseudo-rule though...)

If setting *print-circle* to nil fixes it then do it in xlisp-test - perhaps under a feature test for ECL.

@wobh
Copy link
Contributor Author

wobh commented Sep 30, 2015

Let me know if you think it's too clever.

wobh@1982134

I also added a bunch of tests.

@wobh
Copy link
Contributor Author

wobh commented Oct 1, 2015

It's definitely ECL's load with the :print option and setting *print-circle* to t makes it work as expected. Reviewing the spec for *print-circle* http://l1sp.org/cl/*print-circle* it seems like it would be fine to set it to t generally, and perhaps even a good idea.

@wobh wobh self-assigned this Oct 1, 2015
@wobh
Copy link
Contributor Author

wobh commented Oct 1, 2015

A passing build: https://travis-ci.org/wobh/xlisp/jobs/83063702

I have two ideas here based on the same foundation.

First add some constants to label the verbosity levels. This takes care of an irksome remainder I decided not to do initially for some reason, but also makes distinguishing the two solutions a easier.

Solution 1. Set *print-circle* to t in the lexical scope of load-package. This is a minimal typing solution and the passing build above is the prime-factors exercise rebased onto that branch. At the bottom you can see the circular list printed correctly. The rationale here is that whatever the load outputs, it should be safe, so just set it to t and be done.

Solution 2. Set *print-circle* to the outcome of (verbosity-p +debug+) in the lexical scope of load-package and set the load :print argument to the same. This is a minimal effect solution, observing that the extra output of load with :print is not strictly desirable for general testing, and *print-circle* doesn't need to be any value not required.

I prefer solution 1 for the time being, but I think considering what solution 2 does for us is worth thinking about more generally. If/when we find another place to set *print-circle* to t or have to set it's dynamic value in test-exercises we can do so by verbosity level required with verbosity-p.

@verdammelt
Copy link
Member

I agree - it looks like setting *print-circle* to t is a good idea. It is a good thing for a testing harness to do - given that there may be circular data - we'd want the harness to be able to handle and report on that without it having its own problems.

I go with solution 1. If we tied it to verbosity then we should consider the other *print-x* settings which might also be needed/useful. But I think the simpler approach is better for now.

wobh added a commit to wobh/xlisp that referenced this issue Oct 3, 2015
wobh added a commit to wobh/xlisp that referenced this issue Oct 3, 2015
verdammelt added a commit that referenced this issue Oct 4, 2015
@wobh wobh closed this as completed Oct 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants