-
-
Notifications
You must be signed in to change notification settings - Fork 410
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
austin987
wants to merge
2
commits into
Winetricks:master
Choose a base branch
from
austin987:contributing
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
../CONTRIBUTING.md |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
../CONTRIBUTING.md |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
# Contributing to Winetricks | ||
|
||
## License: | ||
Winetricks is licensed under the LGPL 2.1 or later. Sending a pull request indicates your willingness to license your contribution under this license. | ||
|
||
## Coding standards: | ||
* Documented at top of ```src/winetricks``` | ||
* POSIX shell, no bash (maintainer's scripts can be python/bash) | ||
* If something is unclear, ask | ||
|
||
## Making a patch: | ||
* Check out the source: ```git clone [email protected]:Winetricks/winetricks.git``` | ||
* Hack the source: ```vi src/winetricks``` | ||
* Test it: | ||
* Ideally, Winetricks should work under any Wine version. In practice, testing against the current development and stable versions is sufficient. | ||
* If a bug is only present in some Wine versions, w_workaround_bug() should be used | ||
* ```./src/winetricks -q -v foo``` | ||
* ```./tests/shell-checks```: MUST pass (Travis CI verifies) | ||
* 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 | ||
|
||
## Sending your patch: | ||
* Commit: | ||
* Commit should start with component affected, or misc/winetricks if generic, followed by a short summary: | ||
```git commit -a -m 'vcrun2005: fix a typo'``` | ||
* If you add a new verb, use the format: `verb_name: new verb(, description...)` | ||
* Extended git logs are okay if more explanation is needed | ||
* Send PR: https://github.com/Winetricks/winetricks/compare/ | ||
* If you are asked for changes: | ||
* Edit the source/commit as appropriate: ```vi src/winetricks``` / ```git commit --amend -a``` | ||
* Force push ```git push -f your_org your_repo``` | ||
* Github will automatically update the Pull Request, but it WON'T send a notification. | ||
* Make sure to comment/tag maintainer so he knows there's been an update | ||
|
||
## Bug reports: | ||
* Bug reports must contain, at a minimum: | ||
* The Winetricks version (printed at top of stdout, or use ```winetricks --version```) | ||
* The Wine version used (```wine --version```) | ||
* The failing command (```winetricks foobar```) | ||
* The terminal output, preferably as a ```.txt``` attachment | ||
* Bug reports lacking this information may be closed without warning | ||
|
||
* Feature requests should provide as much detail as possible, along with the tested Winetricks version |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll let you figure that out :)
Done: #891
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 :)