[web catalog migration] define compile-time variables in .spec

[nephros]

To prepare for a possible move of the Web Catalog infrastructure, make the core variables configurable/defined in the .spec file

[Olf0]

Looks fine for me, now that I looked at the code changes.

I should have done that in the first place, instead of replying right after reading your comment above. Hence I minimised ("hid") my original comment, because it turned out to be "off topic", although it still may be helpful; it was written under the false assumption that this was a preparatory step for merging the code of the Django-based Web Catalog web-app here.

[Olf0]

@nephros, please revert my commit 219bc17 if it was wrong to eliminate the C header file define # MEDIA_URL SERVER_URL "/" MEDIA_PATH copied as a comment here in the spec file. It looked unrelated and superfluous / potentially confusing to me here.

Side note: Checking these in src/bin/patchmanager-daemon/patchmanagerobject.h (and as a verbatim copy in src/qml/webcatalog.h) …

# SERVER_URL          "https://coderus.openrepos.net"
# API_PATH            "pm2/api"
# PROJECTS_PATH       "projects"
# PROJECT_PATH        "project"
# RATING_PATH         "rating"
# FILES_PATH          "files"
# MEDIA_PATH          "media"
# CATALOG_URL         SERVER_URL "/" API_PATH
# MEDIA_URL           SERVER_URL "/" MEDIA_PATH

… made me wonder, if there is a path element pm2/ missing from PROJECTS_PATH and PROJECT_PATH, plus maybe also from RATING_PATH, FILES_PATH and MEDIA_PATH?
Because as a matter of fact PROJECTS_URL="${SERVER_URL}/pm2/$PROJECTS_PATH" and PROJECT_URL="${SERVER_URL}/pm2/$PROJECT_PATH$PROJECT"! I have not checked the other three paths.

What do you think?
Apparently PM's Web Catalog works, but these defines seem wrong!?!

[nephros]

… made me wonder, if there is a path element pm2/ missing from PROJECTS_PATH and PROJECT_PATH, plus maybe also from RATING_PATH, FILES_PATH and MEDIA_PATH?

No, as they are used like e.g. at https://github.com/sailfishos-patches/patchmanager/blob/master/src/bin/patchmanager-daemon/patchmanagerobject.cpp#L1923:

 QUrl url(QStringLiteral(CATALOG_URL "/" PROJECTS_PATH));

so if anything they should have an /api too much. But it turns out there's normal web browsing at /pm2/project/foo, and JSON (or other) responses at /pm2/api/project/foo:

• https://coderus.openrepos.net/pm2/projects/
• https://coderus.openrepos.net/pm2/api/projects

[nephros]

@nephros, please revert my commit 219bc17 if it was wrong to eliminate the C header file define # MEDIA_URL SERVER_URL "/" MEDIA_PATH copied as a comment here in the spec file. It looked unrelated and superfluous / potentially confusing to me here.

Well, the media path is actually a different beast than the api locations.
It's basically where the files are downloaded from (which the web catalog software allows to be somewhere different than the catalog itself).

It's fine to remove from the comments as you did, as we don't mention it otherwise in the .spec, but that define should probably become configurable in the spec as well.

[Olf0]

I don't know why this MR threw a notification for me, as I cannot detect any status change etc.

Anyway, I reread our old conversation, and as always you answered all my checks&balances-questions exhaustively. I suppose that is why I marked it as reviewed.

Now, with some newly gathered spec file know-how, I might ask the nitpicking question, why you decided not to specify all three new defines as %global:

• Generally defines via %global are preferred today, because …
• %global defines are evaluated once, when they are created. In contrast, %defines are evaluated each time they are used; hence the are necessary when variables / other defines may change during while a spec file is worked through (by RPM, libzypp etc.), but only then.
• %global defines can be used in all sections of a spec file, but a %define in the header section is not valid in any of the scriptlets (%pre, %post, %preun, %postun, %pretrans, %posttrans and the %filetrigger…-scriplets).

Thus you may consider to change the only %define also to %global, if I did not miss an aspect.
Or not, because this is just a suggestion (from me) / general guidance (from the RPM maintainers / RedHat / Fedora, with a large overlap between these three).

HTH

[Olf0]

IMO this is ready to be merged.