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

Fix: windows installer #1394

Closed
wants to merge 5 commits into from
Closed

Fix: windows installer #1394

wants to merge 5 commits into from

Conversation

mostafa
Copy link
Member

@mostafa mostafa commented Apr 14, 2020

Hi,

I have added the new k6 logo plus its icon and banners to the windows installer. Upon getting the code signing certificate, I'll fix the package signing across all distributions, especially #1034 and #1247.

Also, the current ".ico" file is not used to make a k6 executable, so I created a ".syso" file using rsrc tool. The go build process picks it up and includes it in the final executable file. so I used goversioninfo to generate a resource file to accompany the go build process.

I don't expect a new release just for the sake of this small change, so this PR can wait for the next release, or you decide.

@mostafa mostafa requested review from na--, imiric and mstoykov April 14, 2020 14:27
@codecov-io
Copy link

codecov-io commented Apr 14, 2020

Codecov Report

Merging #1394 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1394   +/-   ##
=======================================
  Coverage   75.46%   75.46%           
=======================================
  Files         150      150           
  Lines       10911    10911           
=======================================
  Hits         8234     8234           
  Misses       2210     2210           
  Partials      467      467           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19c3a3d...7ee006b. Read the comment docs.

@na--
Copy link
Member

na-- commented Apr 14, 2020

https://github.com/josephspurrier/goversioninfo this also looks like a good tool to furnish other useful data besides the icon file in the .syso file. And when you have a go:generate directive, it offers us a reproducible way to generate the binary .syso file, instead of simply checking in a binary in git.

@mostafa mostafa force-pushed the fix/windows-installer branch from 402878c to 7ee006b Compare April 15, 2020 09:19
@CLAassistant
Copy link

CLAassistant commented Apr 15, 2020

CLA assistant check
All committers have signed the CLA.

@mostafa mostafa force-pushed the fix/windows-installer branch from 7ee006b to 2b49e99 Compare August 8, 2020 23:03
@codecov-commenter
Copy link

codecov-commenter commented Aug 8, 2020

Codecov Report

Merging #1394 into master will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1394      +/-   ##
==========================================
+ Coverage   77.13%   77.16%   +0.03%     
==========================================
  Files         162      162              
  Lines       13263    13263              
==========================================
+ Hits        10230    10234       +4     
+ Misses       2512     2508       -4     
  Partials      521      521              
Impacted Files Coverage Δ
lib/executor/vu_handle.go 93.69% <0.00%> (-1.81%) ⬇️
stats/cloud/collector.go 79.23% <0.00%> (+0.54%) ⬆️
js/runner.go 83.73% <0.00%> (+0.69%) ⬆️
lib/testutils/minirunner/minirunner.go 82.14% <0.00%> (+3.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b83dbbd...8f6be5c. Read the comment docs.

Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

I'd forgotten this PR, and the version bump reminded me 😞 But that same version bump also is a bit worrying to me, given how many times we've forgotten to update such static things (e.g. the Go version in the AppVeyor config), so I'd prefer if we can avoid adding more such opportunities...

Given that we have the k6 version as a code constant: https://github.com/loadimpact/k6/blob/3a713ec1b70b4c79eee6897b78d6dfd7ab659ce5/lib/consts/consts.go#L30-L31

And that we can get even more build details as part of the CI process, if we want to include such information in the windows package: https://github.com/loadimpact/k6/blob/3a713ec1b70b4c79eee6897b78d6dfd7ab659ce5/.circleci/config.yml#L174-L175

I think the approach here should be something like:

  • have a versioninfo.json.template template and a simple Go program in /packaging that imports k6/lib/consts and creates the real versioninfo.json with the actual current k6 version
  • add the resulting packaging/versioninfo.json to .gitignore, so that the VersionDetails doesn't contain -dirty
  • execute go run generate_versioninfo.go on every windows build

Something like that should save us some manual work on every release and, more importantly, guarantee that we can never forget to update the version info accidentally. Whether this has to be done as a part of this PR (i.e. in AppVeyor), or as a part of @imiric's efforts to transition to GitHub actions, I don't know.

@imiric
Copy link
Contributor

imiric commented Nov 12, 2020

Hey guys, what's the status of this? @mostafa Are you still willing to work on it? I can takeover if not.

It will need to be updated to GitHub Actions and implementing that templating proposal by @na--.

@mostafa
Copy link
Member Author

mostafa commented Nov 12, 2020

@imiric It would be awesome if you can take over. I can test it afterward.

@imiric
Copy link
Contributor

imiric commented Nov 18, 2020

Closing this in favor of #1727.

@imiric imiric closed this Nov 18, 2020
@mostafa mostafa deleted the fix/windows-installer branch July 2, 2021 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants