Re: package-inferred-systems and primary-system-name
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
Re: package-inferred-systems and primary-system-name
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
Re: package-inferred-systems and primary-system-name
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• http://fare.tunes.org
The trouble with opportunity is that it always comes disguised as hard work.
— Herbert V. Prochnow
Re: package-inferred-systems and primary-system-name
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
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
