package-inferred-systems and primary-system-name
If a have a package-inferred-systems "a" and "a/b/c", the following code used to return "a": (primary-system-name (find-system "a/b/c")) But after commit 069cd2a6 it returns nil. Happy to patch it, but I wanted to check how to do it before starting. The root cause right now is that system-source-file returns nil for the inferred systems. The easiest fix would be to have it return the asd file for the primary system. That's probably not *technically* correct since the system isn't actually defined in that file, but it's probably correct in the principle of least surprise sense. Thoughts? -Eric
On 28 Feb 2018, at 15:46, Eric Timmons wrote:
If a have a package-inferred-systems "a" and "a/b/c", the following code used to return "a":
(primary-system-name (find-system "a/b/c"))
But after commit 069cd2a6 it returns nil.
Happy to patch it, but I wanted to check how to do it before starting. The root cause right now is that system-source-file returns nil for the inferred systems. The easiest fix would be to have it return the asd file for the primary system. That's probably not *technically* correct since the system isn't actually defined in that file, but it's probably correct in the principle of least surprise sense.
Thoughts?
Looking over the change, I believe the issue is that, unfortunately, the "/" character is being used for two different purposes. In the "slashy" systems, it is used to identify subsystems, but the use in package-inferred systems is subtly different, because the location of their definitions as systems is different (indeed the package-inferred systems don't *have* explicit definitions). Also, I don't know that multiple slashes are ("a/b/c") are really supported in the case of systems that are defined in `a.asd`, but I haven't checked. I agree with you, though, that it's reasonable to treat a package-inferred system "a/b/c" as having "a" as its primary system name. I believe that this is the diff that caused the change in that commit: ``` - (component (primary-system-name (coerce-name (component-system system-designator)))))) + (component (let* ((system (component-system system-designator)) + (source-file (physicalize-pathname (system-source-file system)))) + (and source-file + (equal (pathname-type source-file) "asd") + (pathname-name source-file)))))) ``` But I confess that I don't know the rationale for that change, so I don't know what collateral damage there will be to changing it. If you are going to patch this, will you please make a test case? I believe it could be easily added to package-inferred-system-test.script. I will be happy to help, if you would like. It looks like you could add a separate branch to the `etypecase` with the logic special to `package-inferred-system`. If you have access to the cl.net gitlab, then a merge request would be a great way to supply a patch, but if that doesn't work for you, sending a git patch or just a regular old diff patch would also be fine. Thanks for spotting this, best, r
Dear Eric, Robert,
:Eric If a have a package-inferred-systems "a" and "a/b/c", the following code used to return "a":
(primary-system-name (find-system "a/b/c"))
But after commit 069cd2a6 it returns nil.
Thanks for finding that bug. Sorry we didn't have regression tests for that.
The root cause right now is that system-source-file returns nil for the inferred systems. The easiest fix would be to have it return the asd file for the primary system. That's probably not *technically* correct since the system isn't actually defined in that file, but it's probably correct in the principle of least surprise sense.
This is probably technically correct enough, since that's the file that, when it was loaded, implicitly defined the system, and that, if modified, is likely to cause the system to be redefined. The "obvious" solution is then that in package-inferred-system.lisp, instead of :source-file nil the defsystem form in sysdef-package-inferred-system-search should say :source-file ,(system-source-file top)
: Robert Looking over the change, I believe the issue is that, unfortunately, the "/" character is being used for two different purposes. In the "slashy" systems, it is used to identify subsystems, but the use in package-inferred systems is subtly different, because the location of their definitions as systems is different (indeed the package-inferred systems don't have explicit definitions). Also, I don't know that multiple slashes are ("a/b/c") are really supported in the case of systems that are defined in a.asd, but I haven't checked.
I agree with you, though, that it's reasonable to treat a package-inferred system "a/b/c" as having "a" as its primary system name.
I believe that this is the diff that caused the change in that commit:
- (component (primary-system-name (coerce-name (component-system system-designator)))))) + (component (let* ((system (component-system system-designator)) + (source-file (physicalize-pathname (system-source-file system)))) + (and source-file + (equal (pathname-type source-file) "asd") + (pathname-name source-file))))))
But I confess that I don't know the rationale for that change, so I don't know what collateral damage there will be to changing it.
I recommend against changing this function. It plays an important role in supporting systems that are not named after the file they are defined in.
If you are going to patch this, will you please make a test case? I believe it could be easily added to package-inferred-system-test.script. I will be happy to help, if you would like.
Yes, that's the place where a test case would fit.
It looks like you could add a separate branch to the etypecase with the logic special to package-inferred-system.
I think it's better to correctly set the system-source-file.
If you have access to the cl.net gitlab, then a merge request would be a great way to supply a patch, but if that doesn't work for you, sending a git patch or just a regular old diff patch would also be fine.
Eric, will you send a MR? —♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org The trouble with opportunity is that it always comes disguised as hard work. — Herbert V. Prochnow
MR sent. I took the approach of setting the source-file. The etypecase approach would have either introduced a circular dependency between system.lisp and package-inferred-system.lisp or would have required the package-inferred-system symbol to be exported from system.lisp which felt wrong. -Eric
On 1 Mar 2018, at 10:47, Eric Timmons wrote:
MR sent. I took the approach of setting the source-file. The etypecase approach would have either introduced a circular dependency between system.lisp and package-inferred-system.lisp or would have required the package-inferred-system symbol to be exported from system.lisp which felt wrong.
-Eric
Merged into 3.3.1.7 Thanks for the patch! Best, r
participants (3)
-
Eric Timmons -
Faré -
Robert Goldman