search

boutproject / BOUT-dev

BOUT++: Plasma fluid finite-difference simulation code in curvilinear coordinate systems
http://boutproject.github.io/
GNU Lesser General Public License v3.0
179 stars 93 forks source link

Pull out duplicated code from upgrader scripts #2936

Open ZedThree opened 2 weeks ago

ZedThree commented 2 weeks ago

Not sure why I didn't see this solution earlier, it was pretty trivial

ZedThree commented 2 weeks ago

I'm very reluctant to do that because it adds a bunch of overhead both for maintenance and for the user, when these are supposed to be convenience scripts.

Otherwise we have to pull all of these scripts out into a package, with each as a separate entry point, and the user has to create a venv and install the package. This might be easier when there's more tools supporting PEP723, but for the sake of three small functions, I think this is fine

dschwoerer commented 1 week ago

Would you be happy to upload it to pypi, so that I can put it for fedora in site-packages (this can be done in general for installations, which is probably not done often)

That way it works out of the box, it can be properly installed, and boutupgrader cannot be taken over by another project, in order to avoid conflicts.

ZedThree commented 1 week ago

I definitely don't want to upload it to pypi, this is a purely internal library that's only useful for this small set of scripts. Is the issue that you're worried about installing these scripts? Maybe we shouldn't do that then, they have fairly limited use so probably not worth cluttering up people's PATH

dschwoerer commented 1 week ago

On 7/10/24 10:16, Peter Hill wrote:

I /definitely/ don't want to upload it to pypi, this is a purely internal library that's only useful for this small set of scripts.

I do understand the motivation. However, that is in contrast to the best practices [0]

I wander whether it would be best to just move those scripts to boututils?

Than they can be properly packaged and installed more easily.

Is the issue that you're worried about installing these scripts? Maybe we shouldn't do that then, they have fairly limited use so probably not worth cluttering up people's |PATH|

I do think they should be installed, as they are fairly useful for updating to the latest bout++. I do not want users that have installed bout++*-devel via dnf, having to get the source from somewhere to get those scripts.

It would also make it easier to use them, as you don't need to specify the full path, as python's bin folder is normally already in $PATH.

It would also be beneficial for applying those scripts to an older version. Right now, if you have an old branch and want to run the scripts, that is a bit hacky.

[0] https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_pypi_parity

ZedThree commented 2 days ago

That best practices is only for Fedora, and is for standalone Python packages, so it's not super relevant here.

It would also be beneficial for applying those scripts to an older version. Right now, if you have an old branch and want to run the scripts, that is a bit hacky.

I'm not sure that should be a problem really, if your branch is old enough that it doesn't have the upgrader scripts, it probably doesn't need them?

I'm not opposed to moving them all to boututils though. I could imagine then adding each script as a subcommand to a top-level bout-upgrade tool. That would be fairly neat, solve some problems, and keep everyone happy.

dschwoerer commented 2 days ago

On 7/16/24 17:59, Peter Hill wrote:

It would also be beneficial for applying those scripts to an older
version. Right now, if you have an old branch and want to run the
scripts, that is a bit hacky.

I'm not sure that should be a problem really, if your branch is old enough that it doesn't have the upgrader scripts, it probably doesn't need them?

I used them when I tried to update my branch to next, so that I could merge them without to many merge conflicts. But I guess that is not a super common case (and did not work that well due to clang-format being fiddely)

I'm not opposed to moving them all to |boututils| though. I could imagine then adding each script as a subcommand to a top-level |bout-upgrade| tool. That would be fairly neat, solve some problems, and keep everyone happy.

Do you have time to do this, or do you want me to propose a PR?