CMake: prefer placeholders to variables in InnoSetup template

[kmilos]

Also reuse the CPACK_SYSTEM_NAME variable and rename GPHOTO2 ones, they're not NSIS specific any more.

[victoryforce]

Hmm...

prefer placeholders to variables in InnoSetup template

What is this for? What problem does this change solve? Well, I do understand that @VAR@ syntax helps preventing issues with text that might coincidentally look like CMake variables (e.g., $VAR). But this is not the case with Inno Setup script files. This change, of course, doesn't break anything, but it doesn't fix anything either, not even in the sense of making it future-proof.

Also reuse the CPACK_SYSTEM_NAME variable

Also, I see no point in replacing ${ARCH_STRING} with @CPACK_SYSTEM_NAME@. Ultimately, we won't need to support two installation systems, Inno Setup and NSIS. So eventually we'll want to remove NSIS as an outdated system that seems to have stopped developing and never managed to fix the arm64 installation problem. Yes, we will have to test the Inno Setup based installer thoroughly before doing so. But in the end, NSIS won't be needed, so it would be right to remove all the CPACK code supporting it (yeah, along with @CPACK_SYSTEM_NAME@).

and rename GPHOTO2 ones, they're not NSIS specific any more.

That's true. I was going to add other variables later, while polishing the implementation, that wouldn't have CPACK in their names, since CPACK would eventually be removed.

@kmilos You could help a lot without duplicating my work. You are contributing to the MSYS2 project. It would be really cool if you could add the innosetup package there. Now it is not a big problem to install a native Windows installation of innosetup in the runner (via choco, for example) and call it via the pwsh shell. But for users who build darktable themselves, innosetup in msys2 would simplify the process. Yes, I know that in the msys2 shell you can call programs installed on the host system, but still...

[kmilos]

Inno Setup uses {} for its constants. @@ simply stands out and expresses intent unambigously. This best practise is already used for all other templates in our project (and CMake itself).

[victoryforce]

Inno Setup uses {} for its constants.

Yes, exactly. This is always {constant}, never ${constant}. This is exactly what I wrote about in the comment above. Nothing in Inno Setup configuration language may look like a CMake variable. So, yes, your PR is solving a problem that doesn't exist.

@@ simply stands out and expresses intent unambigously.

The dollar-curlybracket form also does not create any ambiguity.

This best practise is already used for all other templates in our project (and CMake itself).

While working on this I've been going through a lot of .iss files on github. I've never seen one with the @@ construct. All of them where the build process used configure_files in CMake had the ${} construct in the .iss template. You use a certain form of variable notation in your project and you think it should be used everywhere, but another form is much more popular.

[victoryforce]

@kmilos what about innosetup in msys2 package base? are you interested in that?

[kmilos]

The dollar-curlybracket form also does not create any ambiguity.

Maybe it's just my eye sight getting worse w/ age, but I strongly disagree.

what about innosetup in msys2 package base? are you interested in that?

Not particularly, sorry. As you mentioned earlier, choco install will cover darktable CI needs.

[victoryforce]

The dollar-curlybracket form also does not create any ambiguity.

Maybe it's just my eye sight getting worse w/ age, but I strongly disagree.

The same. I couldn't disagree more with the promotion of the rarely used (and therefore confusing to many people) "name between @" variable form. Although, if you convince others to replace ${} with @@, I might follow suit so I don't end up being the black sheep. RawTherapee and ART are good projects to start with.

what about innosetup in msys2 package base? are you interested in that?

Not particularly, sorry. As you mentioned earlier, choco install will cover darktable CI needs.

Yes, I know how to do it in nightly. I wanted to make things easier for those users who build the Windows package themselves.

[kmilos]

rarely used (and therefore confusing to many people)

Ok, now I get you've just been joking around all along.

[victoryforce]

Ok, now I get you've just been joking around all along.

No.

And I no longer understand your comments. What is your ultimate goal in forcing me to submit to your views on the best form of representing variables? You are not helping to speed up the transition to the new installer and shipping of arm64 package. You are wasting your time and mine on an unimportant and unnecessary dispute.

[TurboGit]

I know nothing about InnoSetup but I think we shouldn't have such a heated discussion for a change between @@ and ${} especially since both are equivalent if I understand correctly.

My position is I don't really care. Both are fine by me. So please can we find an agreement about what to favor? Or have the main dev dragging the InnoSetup setup (@victoryforce as far as I know) have the last word?

Thanks!

[victoryforce]

@TurboGit Sorry for the pause, I have to go to the hospital to visit my sister, I will reply tonight.

[TurboGit]

Nothing to be sorry about. Your family is top priority compared to darktable project and there is nothing urgent anyway.

[kmilos]

both are equivalent

Technically equivalent, of course, but this was just about UX from the start, and I gave my reason for it - one is simply more readable for me, hence the "preference" in the title. You do realize there are dislexic and otherwise visually impaired people out there as well?

I think we shouldn't have such a heated discussion

I thought so too. Why a small net positive change for some requires such a level of discussion is beyond me.

the main dev

So, one just has to get somewhere first to become one? Ok then.

Vikor, I don't appreciate being pigeonholed, and I resent any assumptions about my motives. I believe I have more then earned the benefit of doubt and some respect around here.

One thing is for sure - I do not have time for this.

[TurboGit]

@kmilos : We have to live with the fact that we have different perceptions. As you said "one is simply more readable for me" and "Why a small net positive change" indeed that's your view but it is not shared by @victoryforce.

Again I have no strong opinion, both are fine by me.

So, one just has to get somewhere first to become one? Ok then.

Well what do you propose :) If it is a bug this is of course not the position I'll take... But for small details like this one which are fully equivalent what can we do? You have a view, Victor has another one and I don't care... What solution should we take then?

I believe I have more then earned the benefit of doubt and some respect around here.

No question about this. That's why I have decided to participate here. Both of you are very valuable devs in the darktable
team. When you contribute I certainly do far less review as I almost blindly know that your contributions are always good and well thought.

Finally... I'm certainly a romantic guy, but the world is so crazy those recent months that I'd like to keep the dev community a peaceful place where everyone can contribute in a respectful manner. We certainly shouldn't have such heated debate for small things like that.

I hope we'll find a positive end to this.

[kmilos]

We have to live with the fact that we have different perceptions

Indeed. So if there is a slightly positive one and a neutral one (Viktor never argued he preferred the other IMHO, just that they're all the same), in a community fostering mutual respect and tolerance, what would one reasonably expect to happen from the start?

I think I'll take a little break here as I no longer feel aligned nor welcome.

[TurboGit]

... as I no longer feel aligned nor welcome.

I just said the opposite!

[kmilos]

@TurboGit I do appreciate your efforts.