Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bitcoin-Qt (Windows only): extend Resource File #1607

Merged
merged 1 commit into from
Aug 17, 2012
Merged

Bitcoin-Qt (Windows only): extend Resource File #1607

merged 1 commit into from
Aug 17, 2012

Conversation

Diapolo
Copy link

@Diapolo Diapolo commented Jul 18, 2012

bitcoin-qt.exe meta information

@laanwj
Copy link
Member

laanwj commented Jul 18, 2012

Hm, nice to add some metadata, but it adds yet another place to update the version number. That sucks... Maybe only add the major versions?

@Diapolo
Copy link
Author

Diapolo commented Jul 18, 2012

I guess this could be scripted in the Gitian process, but I'm no Linux-Script guy. I would love to see this added and I'm fine with major version numbers (e.g. 0.7.X.0), but could take a look here and update when needed :).

It's also possible to include a header with that version stuff, but it's only allowed to consist of the version #defines. So our version.h did not work, too bad.

@laanwj
Copy link
Member

laanwj commented Jul 19, 2012

"makes bitcoin-qt.exe a bit more trust-worthy"
that's an interesting claim :-)

@Diapolo
Copy link
Author

Diapolo commented Jul 19, 2012

@laanwj Why? When I use the tool ProcessExplorer, which is able to show some internal informations of running processes, handles and so on, I consider an application that provides no meta-data as suspicious on the first look. I think it's a good style to supply such meta-data.

@laanwj
Copy link
Member

laanwj commented Jul 19, 2012

Heh, you meant "makes bitcoin-qt.exe look more trust-worthy". Actually becoming more trust-worthy... would be interesting, that was the joke.

I'm for merging this of course. Can't we remove the version completely, for now, until someone sets up a script to insert it automatically?

@Diapolo
Copy link
Author

Diapolo commented Jul 19, 2012

I meant look more trust-worthy, which is a personal thing, so yes ;).

Well I could simply comment out the version #defines but I fear no Linux dev who is able to write shell scripts is willing to put work into this ^^. I'm going to take another look at this issue now.

Edit: Btw. is version.h missing by intent from HEADERS += in the project file?

@grue0
Copy link

grue0 commented Jul 19, 2012

@Diapolo

more trustworthy

wow really? that's about as effective as naming bitcoin-qt.exe to legit_bitcoin-qt.exe

VALUE "InternalName", "bitcoin-qt"
VALUE "LegalCopyright", "2009-2012 The Bitcoin developers"
VALUE "LegalTrademarks1", "Distributed under the MIT/X11 software license, see the accompanying"
VALUE "LegalTrademarks2", "file COPYING or http://www.opensource.org/licenses/mit-license.php."
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure there's no point to breaking up the message into different properties.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm allowed to use 2 rows and I chose the exact format used all over the source, I think that is just fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm with gruez on this one. LegalCopyright is a separate unit, but I don't see why split LegalTradeMarks1 / LegalTradeMarks2 at a illogical place between two words.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merged LegalTrademarks2 into LegalTrademarks1 :), you convinced me.

@Diapolo
Copy link
Author

Diapolo commented Jul 19, 2012

I split the needed version stuff into versionrc.h ... still a manual solution, but a small step perhaps to one who can help automating this :-D.

@Diapolo
Copy link
Author

Diapolo commented Jul 26, 2012

Updated FileDescription string and I was able to change the language property to display neutral language, which is better, because we have quite many translations in.

@BitcoinPullTester
Copy link

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/368e127e53f7dfe517b96f7fb1b365b3ef1c77a3 for binaries and test log.

@laanwj
Copy link
Member

laanwj commented Aug 14, 2012

Can we merge this without the version for now? Just add base metadata for this pull, and do the scripting stuff in a later pull.

@Diapolo
Copy link
Author

Diapolo commented Aug 14, 2012

I can comment out the version stuff in the .rc file and remove the header for now. But to not forget about this I'll re-open a pull after the basic one is in okay?

- extend bitcoin-qt.rc to include meta information, which is displayed on
  Windows, when looking in the executable properties and selecting
  "Details"
- does currently NOT include version information, this is scheduled
  for later releases
- for RC-file documentation see:
  http://msdn.microsoft.com/en-us/library/windows/desktop/aa381058%28v=vs.85%29.aspx
@Diapolo
Copy link
Author

Diapolo commented Aug 14, 2012

Last update removed all version stuff (for now), see the screenshot in the first posting.

@BitcoinPullTester
Copy link

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/4b94f4e0ed38a95a3e2c3e80258fe73b88c3826c for binaries and test log.

@sipa
Copy link
Member

sipa commented Aug 17, 2012

ACK

laanwj added a commit that referenced this pull request Aug 17, 2012
Bitcoin-Qt (Windows only): extend Resource File
@laanwj laanwj merged commit a108d3d into bitcoin:master Aug 17, 2012
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants