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

Version Patch v0.0.2 #202

Merged
merged 1 commit into from
Oct 30, 2016
Merged

Version Patch v0.0.2 #202

merged 1 commit into from
Oct 30, 2016

Conversation

GrayHatter
Copy link

@GrayHatter GrayHatter commented Oct 24, 2016

This change is Reviewable

@GrayHatter
Copy link
Author

suggested by @nurupo because the logger was fixed in 949ef78 that would be a good place to increment the patch version.

@GrayHatter GrayHatter added this to the v0.0.2 milestone Oct 24, 2016
@iphydf
Copy link
Member

iphydf commented Oct 24, 2016

This incorrectly passes tests. The tests are broken, I'll fix them.

@GrayHatter
Copy link
Author

blocked by #204 ping me on merge so I can rebase.

@iphydf
Copy link
Member

iphydf commented Oct 28, 2016

@GrayHatter ping: #204 is merged, please rebase.

@GrayHatter
Copy link
Author

@iphydf done

@nurupo
Copy link
Member

nurupo commented Oct 29, 2016

@GrayHatter at the very least you have missed https://github.com/TokTok/c-toxcore/blob/master/CMakeLists.txt#L9. Please check the coomit that did 0.0.1 version tick and see if anything esle is missing.

@dvor
Copy link
Member

dvor commented Oct 29, 2016

I don't like that version number is spread all over the project. Is it possible to keep it in single place?

People will change version every release, and people will forget to change it somewhere in those files.

Also even having it in multiple places, you cannot simply search for "0.0.1" string as it is broken into separate major, minor, patch components.

@nurupo
Copy link
Member

nurupo commented Oct 29, 2016

@dvor there is already a script for updating versions in all the places, I'm not sure why @GrayHatter is not using it. All you need to do is bump the version in CMakeLists.txt and it will run https://github.com/TokTok/c-toxcore/blob/master/other/version-sync https://github.com/TokTok/c-toxcore/blob/master/CMakeLists.txt#L458 which should update the version number everywhere.


update 'CMakeLists.txt' 's/^\(set(PROJECT_VERSION_MAJOR \"\).*")/\1'$MAJOR'")/'
update 'CMakeLists.txt' 's/^\(set(PROJECT_VERSION_MINOR \"\).*")/\1'$MINOR'")/'
update 'CMakeLists.txt' 's/^\(set(PROJECT_VERSION_PATCH \"\).*")/\1'$PATCH'")/'
Copy link
Member

@nurupo nurupo Oct 29, 2016

Choose a reason for hiding this comment

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

Uh, you are not supposed to call this script manually. It's supposed to be called by cmake. When releasing a new version, you update the version number in the CMakeLists.txt and run cmake, which will update the version everywhere. At least that's how I understand it's supposed to be done.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's correct. And failing to do so should fail travis.

Copy link
Member

Choose a reason for hiding this comment

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

@GrayHatter what I'm trying to say here is that you should remove these 4 lines you have added and it should be good to go.

If you want to be extra careful, you could discard all the changes you have made in this PR, update the version number in cmake and let cmake call the version sync script to update everything.

@GrayHatter
Copy link
Author

Wait... So cmake is now a dependency for autotools?

Lolwut

Okay I'll fix it tonight. Anyone else can feel free to beat me

@iphydf
Copy link
Member

iphydf commented Oct 29, 2016

No, cmake is not a dependency. The source of truth for the version number is in the cmake file, but that is only relevant when changing the number.

@iphydf
Copy link
Member

iphydf commented Oct 30, 2016

:lgtm_strong:


Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@iphydf iphydf merged commit 60e15f1 into TokTok:master Oct 30, 2016
@GrayHatter GrayHatter deleted the ver_patch branch November 19, 2016 07:18
@iphydf iphydf added refactor Refactoring production code, eg. renaming a variable, not affecting semantics and removed code cleanup labels May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactoring production code, eg. renaming a variable, not affecting semantics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants