Re: New "make benchmark" target

[Andrea Corallo]

Pip Cet <pipcet@protonmail.com> writes:

> "Andrea Corallo" <acorallo@gnu.org> writes:
>
>> Pip Cet <pipcet@protonmail.com> writes:
>>
>>> "Andrea Corallo" <acorallo@gnu.org> writes:
>>>
>>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>>
>>>>>> From: Stefan Kangas <stefankangas@gmail.com>
>>>>>> Date: Wed, 15 Jan 2025 14:20:42 -0800
>>>>>> Cc: acorallo@gnu.org, mattiase@acm.org, eggert@cs.ucla.edu,
>>>>>>  emacs-devel@gnu.org
>>>>>>
>>>>>> Pip Cet <pipcet@protonmail.com> writes:
>>>>>>
>>>>>> > "Eli Zaretskii" <eliz@gnu.org> writes:
>>>>>> >
>>>>>> >>> I'm happy to change the scratch/elisp-benchmarks branch in the ways
>>>>>> >>> we've discussed, and it should be merged, but if someone decides to
>>>>>> >>> incrementally solve some of the issues, that, while not very harmful,
>>>>>> >>> would be an inefficient use of resources.
>>>>>> >>
>>>>>> >> Let's hear what Andrea thinks about the issues you reported (let's
>>>>>> >> please discuss them on the bug tracker, not here), and let's take it
>>>>>> >> from there.
>>>>>> >
>>>>>> > Sure, just let me know when we can merge the branch (assuming you meant
>>>>>> > to say that the merge, too, should wait for Andrea's thoughts) or which
>>>>>> > other changes are required!
>>>>>>
>>>>>> Is there anything left to do here that is blocking the merge?
>>>>>
>>>>> I didn't follow the discussions too closely, but if there are no
>>>>> unresolved issues left between Pip and Andrea, I think this can land.
>>>>>
>>>>> Andrea, any objections or comments?
>>>>
>>>> I've no objections, only two questions:
>>>
>>> Thanks!  I'll merge without the admin/elpa2emacs.sh script (which the
>>> questions are about), then?
>>
>> Hi Pip,
>>
>> I'd prefer we finalize the workflow first, otherwise I don't know how to
>> merge in emacs.git the fixes/improvements we make in ELPA.
>
> Okay!
>
>>
>>>> 1- admin/elpa2emacs.sh looks elisp-benchmarks specific, if that's the
>>>>    case perhaps should have a better name?  Otherwise we should probably
>>>>    make it general?
>>>
>>> You're right, it currently hardcodes the "benchmarks/" destination
>>> directory.  My reasoning for this was that other elpa modules would most
>>> likely require different files to be put into different directories, and
>>> so the script would need modifying anyway.  (As git filter-repo isn't
>>> exactly well-behaved, it would probably be easiest to run the script
>>> once for each destination directory).
>>>
>>>> 2- What is the workflow for importing into emacs.git existing changes in
>>>>    elisp-benchmarks?  I tried to run elpa2emacs.sh as suggested but I do
>>>>    get an error (not sure if the suggested invocation is for the initial
>>>>    import, for merging incremental changes or both).
>>>
>>> What kind of error?  There are two problems here:
>>>
>>> 1. we add a remote called elpa2emacs-filtered-elpa, but we don't remove
>>> it automatically.  Re-running elpa2emacs.sh may use the old remote, with
>>> undesirable results.  But removing a remote means interfering with an
>>> existing git repository even more (I destroyed several emacs repos while
>>> adjusting this script).
>>
>> Okay but I guess we can assume elpa2emacs-filtered-elpa is a name that
>> would not be choosen by a developer no?  Otherwise IMO we have to add in
>> the comment instructions on how to re-run elpa2emacs.sh.
>
> You're right.  I'll add the code to remove the remote again.
>
>>> 2. we hardcode origin/master as the Emacs branch we're working on.  I
>>> believe this is forgivable, but it means we can't currently test the
>>> merge case, because origin/master doesn't contain the elisp benchmarks
>>> yet.
>>
>> I've git.sv.gnu.org/srv/git/emacs.git called 'savannah' instead of
>> 'origin' as I've other remotes.  I think we should have a way to specify
>> the remote name.
>
> Hmm.  Wouldn't that be confusing in the case where people use a fresh
> checkout rather than a new worktree?
>
> Let me think about it: I think it might be best to write the script so
> it's run in an existing repo, adds the worktree itself, then removes it
> when it's done.

SGTM

>> Also I think the script should stop if any of the commands fails while I
>> see now it keep on running.
>
> Agreed.
>
> Would it be okay to make this script include a safety prompt?  It
> modifies the git repository in potentially destructive ways,
> particularly if we change it to be run in the main emacs repo...

I think so yes 👍

Thanks

Andrea