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

Documentation needs to list the version of uncrustify that is required for development #783

Closed
tbohn opened this issue Feb 22, 2018 · 4 comments
Assignees

Comments

@tbohn
Copy link
Contributor

tbohn commented Feb 22, 2018

  • VIC Version: 5.0
  • Compiler: gcc 5.4.0
  • OS: ubuntu
  • Problem: running uncrustify is a required step when developers submit PRs. However, running the wrong version of uncrustify will result in undesirable code changes. On Ubuntu at least, the latest version of uncrustify currently available for a simple installation (v 0.59) is too old, and gives bad results. Upgrading to 0.64 or later fixes the problem (we don't know the exact version that fixes the problem; all we know is that 0.64 is new enough). This required extra effort on my part to explicitly download the source code for a newer version of uncrustify and build it.
  • Proposed solution: we should add a clear statement to the online documentation that uncrustify 0.64 or later is required, if one wants to submit a PR (and fulfill the uncrustify requirement of the PR). It's only necessary for developers, so I guess the statement should go somewhere in the development section.
  • Note also that running uncrustify within the vic_test_env environment MIGHT also be necessary. Haven't tested that for sure. I'll try to find out and update this.
@jhamman jhamman changed the title [Bug] Documentation needs to list uncrustify (with version) as a required dependency Documentation needs to list uncrustify (with version) as a required dependency Feb 23, 2018
@jhamman
Copy link
Member

jhamman commented Feb 23, 2018

@tbohn - uncrustify is not exactly a "required dependency". While it is for developers, it is not for most users so I'm hesitant to elevate its place in the dependency list. That said, I think we should add a few notes about the traditional development environment to this page: http://vic.readthedocs.io/en/master/Development/Contributing/

@dgergel dgergel self-assigned this Feb 23, 2018
@dgergel
Copy link
Contributor

dgergel commented Feb 23, 2018

I can take care of this when I get back (I'm out tomorrow and this weekend)

@tbohn tbohn changed the title Documentation needs to list uncrustify (with version) as a required dependency Documentation needs to list the version of uncrustify that is required for development Feb 23, 2018
@tbohn
Copy link
Contributor Author

tbohn commented Feb 23, 2018

OK, I've changed the title of the issue to not mention required dependency. But the version of uncrustify still needs to be specified.

@dgergel
Copy link
Contributor

dgergel commented May 31, 2018

Closed via #796

@dgergel dgergel closed this as completed May 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants