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

CONTRIBUTING.md: initial checkin #871

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

austin987
Copy link
Contributor

This is an initial attempt at adding a contributing document. I would very much appreciate feedback from anyone who contributes and was confused by any part of the process.

CC'ing people who have contributed recently for their feedback:
@jre-wine @mihaitodor @mmengden @AndreRH @glintwine @AsciiWolf @hillwoodroc @qwertychouskie @Chiitoo @eli-schwartz @kakurasan

All feedback welcome. I plan to leave this open for at least a couple weeks to allow time for feedback.

@austin987
Copy link
Contributor Author

You can see a rendered version here:
https://github.com/austin987/winetricks/blob/contributing/CONTRIBUTING.md

@austin987 austin987 force-pushed the contributing branch 2 times, most recently from df25511 to 548eff1 Compare November 15, 2017 23:44
Copy link
Contributor

@mihaitodor mihaitodor left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for putting this document together! I left some small comments.

CONTRIBUTING.md Outdated
* The terminal output, preferably as a ```.txt``` attachment
* Bug reports lacking this information may be closed without warning

* Feature requests should provided as much detail as possible, along with the tested Winetricks version
Copy link
Contributor

Choose a reason for hiding this comment

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

Small typo: "provided"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, thanks. Will fix soon.

CONTRIBUTING.md Outdated
## Making a patch:
* Check out the source: ```git clone git@github.com:Winetricks/winetricks.git```
* Hack the source: ```vi src/winetricks```
* Test it: ```./src/winetricks -q -v foo```
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to mention that it should be tested against the latest version of Wine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, winetricks isn't tied to a particular wine version (that's why there is w_workaround_wine_bug and friends), but yeah, a comment explaining more is a good idea, thanks.

@kakurasan
Copy link
Contributor

I would like to add the following items ([1], [2], [3] and [4]):

## Making a patch:
* `./tests/shell-checks`: MUST pass (Travis CI verifies)
    * [1] This tool uses checkbashisms (package `devscripts` on Debian-based distributions) and [ShellCheck](https://github.com/koalaman/shellcheck)
    * [2] If ShellCheck fails, see [ShellCheck wiki](https://github.com/koalaman/shellcheck/wiki) and fix/[ignore](https://github.com/koalaman/shellcheck/wiki/Ignore) the error(s)
        * [3] Detailed error information is available in the wiki (e.g. [SC2154](https://github.com/koalaman/shellcheck/wiki/SC2154))

## Sending your patch:
* Commit:
    * Commit should start with ...
        * [4] If you add a new verb, use the format: `verb_name: new verb(, description...)`

@austin987
Copy link
Contributor Author

I've addressed the feedback given so far by @mihaitodor and @kakurasan

@jre-wine
Copy link
Contributor

jre-wine commented Dec 3, 2017

Thanks Austin! Just minor nitpicks (for reference, my PR is at austin987#1, but discussion should probably happen here):

  • in the "Test it" section maybe add some explanation what the tests do (see PR)
  • See PR for a minor rewording about the start of commit messages. I think this makes the sentence easier to read.
  • What are "Extended git logs"? I guess long commit messages. Do we have to specify how long the first line of the commit message may be, and that the second line should be blank?
  • My contributions are not the typical ones, they are often very general and affecting many files. So I often wonder if I should split up my changes (like for patches at winehq.org), or keep everything in one commit. I tend to do the latter here. See PR.

@eli-schwartz
Copy link
Contributor

Looks okay, but consider implementing that instead/also as .github/{ISSUE,PULL_REQUEST}_TEMPLATE.md

* This tool uses checkbashisms (package `devscripts` on Debian-based distributions) and [ShellCheck](https://github.com/koalaman/shellcheck)
* If ShellCheck fails, see [ShellCheck wiki](https://github.com/koalaman/shellcheck/wiki) and fix/[ignore](https://github.com/koalaman/shellcheck/wiki/Ignore) the error(s)
* Detailed error information is available in the wiki (e.g. [SC2154](https://github.com/koalaman/shellcheck/wiki/SC2154))
* ```./tests/winetricks-test check```: optional but recommended, if you have the time and hard drive space this should be run
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean check-deps?

Also, this script seems to assume that winetricks is located in the current folder so this command won't work.

Also, it keeps prompting about installing Mono and Gecko, which the user needs to cancel manually. Maybe this script should set WINEDLLOVERRIDES="mscoree,mshtml="?

Should I open a bug for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, I was thinking of make check. I'll fix later, on mobile.

Re winetricks in other folder, please file an issue. I'm aware of it (didn't write it), been too lazy to fix. But if someone else has noticed, I should ;)

Re mono/gecko, the lack of them isn't considered a valid setup. I'd consider that a bad setup. That said, it would fail on a clean setup (no ~/.cache/wine and no distro gecko/mono package), but I'm not sure if a good fix, other than manually downloading the files before, but they change with wine versions, and would quickly be a mess.

Maybe warning the user first via terminal would be a good compromise?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking of make check

OK, I'll let you figure that out :)

Re winetricks in other folder, please file an issue

Done: #891

Re mono/gecko, the lack of them isn't considered a valid setup.

Right... For my use case, I don't need them at all (and Wine is kind enough to not fault me for not installing them), so I wrongly assumed that these tests might not need them either.

I guess the main issue here is that, since they are needed, Winetricks should be able to automate their download and install. It totally sucks if the user needs an UI to click a "yes" button when running automated tests. Think of having a Jenkins setup where winetricks-test is run automatically after every commit to master. I saw there was some other installer executed during the tests, that was somehow fully automated, even if it showed an UI, so I'm thinking that it should be possible to do the same in this case. Haven't had time to look into it in detail, though.

Related to this, do you have a rough estimate for the amount of disk space needed to run these tests? I tried in a VM where I had about 20 GB to spare and that wasn't enough. I think it would be a good idea to leave a number in this file, like ~50GB or ~100GB etc :)

@mihaitodor mihaitodor mentioned this pull request Dec 7, 2017
@austin987 austin987 force-pushed the contributing branch 2 times, most recently from af9644c to 57012ef Compare December 14, 2017 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants