Tests now passing on all implementations, and there's been substantial clean-up.
Likely the first two commits on this branch should be merged with the plan branch directly, since they don't have anything to do with read dependencies per se. Then we could move on to reviewing (and then squash-merging) read-depends.
Cheers, r
1- At the end of find-system, the methods on additional-input-files should go now that you use the convenience methods, shouldn't they? 2- now that you invalidate the cache for input-files, shouldn't you put the append *inside* the with-asdf-cache? Or would that fail?
—♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org If you wish to examine a granfalloon, just remove the skin of a toy balloon. — Bokonon
On Fri, Jul 14, 2017 at 10:09 AM, Robert Goldman rpgoldman@sift.net wrote:
Tests now passing on all implementations, and there's been substantial clean-up.
Likely the first two commits on this branch should be merged with the plan branch directly, since they don't have anything to do with read dependencies per se. Then we could move on to reviewing (and then squash-merging) read-depends.
Cheers, r
On 7/14/17 Jul 14 -11:17 AM, Faré wrote:
1- At the end of find-system, the methods on additional-input-files should go now that you use the convenience methods, shouldn't they? 2- now that you invalidate the cache for input-files, shouldn't you put the append *inside* the with-asdf-cache? Or would that fail?
I'll get on those.
1. what's in FIND-SYSTEM is accessors to %ADDITIONAL-INPUT-FILES, not ADDITIONAL-INPUT-FILES and the former has setters as well as getters, so I wasn't sure I could use ADDITIONAL-INPUT-FILES. I'll check. 2. I'll fix this, thanks.
I'm going to push the first 2 commits into plan, since that's where they belong, also.
More soon
R
—♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org If you wish to examine a granfalloon, just remove the skin of a toy balloon. — Bokonon
On Fri, Jul 14, 2017 at 10:09 AM, Robert Goldman rpgoldman@sift.net wrote:
Tests now passing on all implementations, and there's been substantial clean-up.
Likely the first two commits on this branch should be merged with the plan branch directly, since they don't have anything to do with read dependencies per se. Then we could move on to reviewing (and then squash-merging) read-depends.
Cheers, r
On 7/14/17 Jul 14 -11:20 AM, Robert Goldman wrote:
On 7/14/17 Jul 14 -11:17 AM, Faré wrote:
1- At the end of find-system, the methods on additional-input-files should go now that you use the convenience methods, shouldn't they? 2- now that you invalidate the cache for input-files, shouldn't you put the append *inside* the with-asdf-cache? Or would that fail?
I'll get on those.
- what's in FIND-SYSTEM is accessors to %ADDITIONAL-INPUT-FILES, not
ADDITIONAL-INPUT-FILES and the former has setters as well as getters, so I wasn't sure I could use ADDITIONAL-INPUT-FILES. I'll check.
Checked and confirmed this -- %ADDITIONAL-INPUT-FILES takes only a COMPONENT as argument, so it can't use the convenience methods macro.
- I'll fix this, thanks.
I don't fully understand. I'm looking, and the APPEND call is in the scope of DO-ASDF-CACHE, AFAICT:
;; Memoize input files. (defmethod input-files :around (operation component) (do-asdf-cache `(input-files ,operation ,component) ;; get the additional input files, if any (append (call-next-method) ;; must come after the first, for other code that ;; assumes the first will be the "key" file (additional-input-files operation component))))
If that's all, I can merge this into the plan branch, and then we should fix the ABCL issues.
Best, r
I'm going to push the first 2 commits into plan, since that's where they belong, also.
More soon
R
—♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org If you wish to examine a granfalloon, just remove the skin of a toy balloon. — Bokonon
On Fri, Jul 14, 2017 at 10:09 AM, Robert Goldman rpgoldman@sift.net wrote:
Tests now passing on all implementations, and there's been substantial clean-up.
Likely the first two commits on this branch should be merged with the plan branch directly, since they don't have anything to do with read dependencies per se. Then we could move on to reviewing (and then squash-merging) read-depends.
Cheers, r
- what's in FIND-SYSTEM is accessors to %ADDITIONAL-INPUT-FILES, not
ADDITIONAL-INPUT-FILES and the former has setters as well as getters, so I wasn't sure I could use ADDITIONAL-INPUT-FILES. I'll check.
Checked and confirmed this -- %ADDITIONAL-INPUT-FILES takes only a COMPONENT as argument, so it can't use the convenience methods macro.
Why do you need convenience methods for the internal accessor %ADDITIONAL-INPUT-FILES ? Can't users who need convenience methods (including internal functions) just use ADDITIONAL-INPUT-FILES, and let *its* convenience methods do the trick?
- I'll fix this, thanks.
I don't fully understand. I'm looking, and the APPEND call is in the scope of DO-ASDF-CACHE, AFAICT:
;; Memoize input files. (defmethod input-files :around (operation component) (do-asdf-cache `(input-files ,operation ,component) ;; get the additional input files, if any (append (call-next-method) ;; must come after the first, for other code that ;; assumes the first will be the "key" file (additional-input-files operation component))))
Oh, right. Somehow I misread that.
If that's all, I can merge this into the plan branch, and then we should fix the ABCL issues.
Yes, it otherwise looks good.
I have a few tweaks to do afterwards: moving the 3.3.0 number past your patches, some comment tweaks I have locally, and pushing back the schedule of warnings to a further release (keep issuing only style-warnings for deprecated functions, for now).
What about merging plan into master after I do that?
—♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org Any sufficiently unadvanced science fiction is indistinguishable from fantasy.
On 7/16/17 Jul 16 -10:31 PM, Faré wrote:
- what's in FIND-SYSTEM is accessors to %ADDITIONAL-INPUT-FILES, not
ADDITIONAL-INPUT-FILES and the former has setters as well as getters, so I wasn't sure I could use ADDITIONAL-INPUT-FILES. I'll check.
Checked and confirmed this -- %ADDITIONAL-INPUT-FILES takes only a COMPONENT as argument, so it can't use the convenience methods macro.
Why do you need convenience methods for the internal accessor %ADDITIONAL-INPUT-FILES ? Can't users who need convenience methods (including internal functions) just use ADDITIONAL-INPUT-FILES, and let *its* convenience methods do the trick?
I was just being lazy, but I definitely found that the raw accessor was being called with strings. Our code structure doesn't make it easy to be tidy about this. The very presence of the action convenience files encourages being haphazard about typing, and the fact that CL gives us next to no compile time support makes it worse. Even if I track down all the callers and do the type correction myself, the next time someone calls it (since we have effectively told the programmer "it's ok to use component and action designators everywhere" by having arguments called ACTION and COMPONENT that actually mean ACTION-DESIGNATOR and COMPONENT-DESIGNATOR), we could be back in the soup.
I can fix this, but I think its symptomatic of a real maintenance issue.
- I'll fix this, thanks.
I don't fully understand. I'm looking, and the APPEND call is in the scope of DO-ASDF-CACHE, AFAICT:
;; Memoize input files. (defmethod input-files :around (operation component) (do-asdf-cache `(input-files ,operation ,component) ;; get the additional input files, if any (append (call-next-method) ;; must come after the first, for other code that ;; assumes the first will be the "key" file (additional-input-files operation component))))
Oh, right. Somehow I misread that.
If that's all, I can merge this into the plan branch, and then we should fix the ABCL issues.
Yes, it otherwise looks good.
OK, I'll get %ADDITIONAL-INPUT-FILES seen to, and review the documentation, and then we can merge.
I'll look into ABCL as time permits.
I have a few tweaks to do afterwards: moving the 3.3.0 number past your patches, some comment tweaks I have locally, and pushing back the schedule of warnings to a further release (keep issuing only style-warnings for deprecated functions, for now).
What about merging plan into master after I do that?
—♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org Any sufficiently unadvanced science fiction is indistinguishable from fantasy.
On Mon, Jul 17, 2017 at 9:47 AM, Robert Goldman rpgoldman@sift.net wrote:
On 7/16/17 Jul 16 -10:31 PM, Faré wrote:
- what's in FIND-SYSTEM is accessors to %ADDITIONAL-INPUT-FILES, not
ADDITIONAL-INPUT-FILES and the former has setters as well as getters, so I wasn't sure I could use ADDITIONAL-INPUT-FILES. I'll check.
Checked and confirmed this -- %ADDITIONAL-INPUT-FILES takes only a COMPONENT as argument, so it can't use the convenience methods macro.
Why do you need convenience methods for the internal accessor %ADDITIONAL-INPUT-FILES ? Can't users who need convenience methods (including internal functions) just use ADDITIONAL-INPUT-FILES, and let *its* convenience methods do the trick?
I was just being lazy, but I definitely found that the raw accessor was being called with strings. Our code structure doesn't make it easy to be tidy about this.
The function didn't exist before, you are introducing it. Why do you call it with strings? Shouldn't this function always be hidden behind a call to ADDITIONAL-INPUT-FILES, that does have convenience methods???
The very presence of the action convenience files encourages being haphazard about typing, and the fact that CL gives us next to no compile time support makes it worse. Even if I track down all the callers and do the type correction myself, the next time someone calls it (since we have effectively told the programmer "it's ok to use component and action designators everywhere" by having arguments called ACTION and COMPONENT that actually mean ACTION-DESIGNATOR and COMPONENT-DESIGNATOR), we could be back in the soup.
I can fix this, but I think its symptomatic of a real maintenance issue.
Convenience functions are fine for API functions; just not for internals that ought to be hidden behind API functions anyway.
—♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org I don't count the word "god" as sacred, so I don't think religions that use it are worse than religions that don't. — Faré
On 7/17/17 Jul 17 -9:40 AM, Faré wrote:
On Mon, Jul 17, 2017 at 9:47 AM, Robert Goldman rpgoldman@sift.net wrote:
On 7/16/17 Jul 16 -10:31 PM, Faré wrote:
- what's in FIND-SYSTEM is accessors to %ADDITIONAL-INPUT-FILES, not
ADDITIONAL-INPUT-FILES and the former has setters as well as getters, so I wasn't sure I could use ADDITIONAL-INPUT-FILES. I'll check.
Checked and confirmed this -- %ADDITIONAL-INPUT-FILES takes only a COMPONENT as argument, so it can't use the convenience methods macro.
Why do you need convenience methods for the internal accessor %ADDITIONAL-INPUT-FILES ? Can't users who need convenience methods (including internal functions) just use ADDITIONAL-INPUT-FILES, and let *its* convenience methods do the trick?
I was just being lazy, but I definitely found that the raw accessor was being called with strings. Our code structure doesn't make it easy to be tidy about this.
The function didn't exist before, you are introducing it. Why do you call it with strings? Shouldn't this function always be hidden behind a call to ADDITIONAL-INPUT-FILES, that does have convenience methods???
The call chain that led to my problem was as follows:
PARSE-DEFSYSTEM > PARSE-COMPONENT-FORM > NORMALIZE-VERSION -- called with COMPONENT (!) a string RECORD-ADDITIONAL-SYSTEM-INPUT-FILE -- component a string %ADDITIONAL-INPUT-FILES ---> FAIL
I've fixed that now... As soon as I've finished all the tests, I will push the patch. Then we can merge. I believe we can squash merge to simplify the history, unless you think we need it...
Best, r
On Mon, Jul 17, 2017 at 2:30 PM, Robert Goldman rpgoldman@sift.net wrote:
The call chain that led to my problem was as follows:
PARSE-DEFSYSTEM > PARSE-COMPONENT-FORM > NORMALIZE-VERSION -- called with COMPONENT (!) a string RECORD-ADDITIONAL-SYSTEM-INPUT-FILE -- component a string %ADDITIONAL-INPUT-FILES ---> FAIL
I've fixed that now... As soon as I've finished all the tests, I will push the patch. Then we can merge. I believe we can squash merge to simplify the history, unless you think we need it...
Looks good to me. Yes, at this point, squashing the commits would be nice.
—♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org Aggression is simply another name for government. Aggression, invasion, government are interchangeable terms. The essence of government is control, or the attempt to control. He who attempts to control another is a governor, an aggressor, an invader; — Benjamin Tucker
On 7/17/17 Jul 17 -5:26 PM, Faré wrote:
On Mon, Jul 17, 2017 at 2:30 PM, Robert Goldman rpgoldman@sift.net wrote:
The call chain that led to my problem was as follows:
PARSE-DEFSYSTEM > PARSE-COMPONENT-FORM > NORMALIZE-VERSION -- called with COMPONENT (!) a string RECORD-ADDITIONAL-SYSTEM-INPUT-FILE -- component a string %ADDITIONAL-INPUT-FILES ---> FAIL
I've fixed that now... As soon as I've finished all the tests, I will push the patch. Then we can merge. I believe we can squash merge to simplify the history, unless you think we need it...
Looks good to me. Yes, at this point, squashing the commits would be nice.
squash merge is done. Now we need to figure out what's going on with ABCL, and I need to figure out how to re-invigorate my windows VM and restart testing there.
R
-
Faré -
Robert Goldman