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

Bring back autotools instructions #376

Merged
merged 1 commit into from
Jan 5, 2017

Conversation

zetok
Copy link

@zetok zetok commented Jan 2, 2017

Reverted, since apparently cmake is not supposed to be used for things
other than testing/development and causes client build failures.

Apparently making it work for clients would require complicating
maintenance, which clearly can't be done.

This reverts commit 48ddb11.


This change is Reviewable

@zetok
Copy link
Author

zetok commented Jan 2, 2017

cc @Encrypt @nurupo could you revert toxcore to autotools on jenkins?

@jin-eld
Copy link

jin-eld commented Jan 2, 2017

As a side note: when you add cmake compiler options or #defines that are supposed to be controllable by the user, let me know or better, add a new issue and assign it to me. I will make sure to add those things to autotools so that both build systems stay in sync in terms of option configuration.

@cebe
Copy link
Member

cebe commented Jan 2, 2017

apparently cmake is not supposed to be used for things
other than testing/development and causes client build failures.

@zetok can you give more details about the issues it caused? maybe they are solveable?

@robinlinden
Copy link
Member

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


INSTALL.md, line 466 at r1 (raw file):

You can also use a precompiled win32 binary of libsodium, however you will have to place the files in places where they can be found, i.e., dll's go to /bin headers to /include and libraries to /lib directories in your MinGW shell.

Next, install toxcore library, should either clone this repo by using git, or just download a [zip of current Master branch](https://github.com/irungentoo/toxcore/archive/master.zip) and extract it somewhere.

Why would we tell people to download irungentoo's toxcore on the TokTok repo?


Comments from Reviewable

@zetok
Copy link
Author

zetok commented Jan 2, 2017

@cebe there were relevant logs on jenkins, now they're probably gone.

According to @iphydf it's not possible to have a maintainable toxcore and usable cmake build at the same time.

I don't really know, I just updated docs to reflect that.


Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


INSTALL.md, line 466 at r1 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

Why would we tell people to download irungentoo's toxcore on the TokTok repo?

Whops, revert gone wild. Fixed.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Jan 2, 2017

cc @Encrypt @nurupo could you revert toxcore to autotools on jenkins?

I could, it would be just a huge pain since it would break Toxic and uTox builds, as they use cmake toxcore, so it would require coordination from @JFreegman and @GrayHatter.

So, before switching Jenkins's toxcore-toktok to autotools, explain what exactly doesn't work in cmake and why shouldn't we just fix it and continue use cmake on Jenkins?

@zetok
Copy link
Author

zetok commented Jan 2, 2017

@nurupo since you seem to have forgotten what doesn't work: https://github.com/qTox/qtox-irc-logs/blob/4dcc37baf6eea28b5ca19ba5be86243b359b630e/2016/12/%23qtox_20161221.log.txt#L46

@tux3 hit the same issue yesterday.

So, before switching Jenkins to autotools, explain what exactly doesn't work in cmake and why shouldn't we just fix it and continue use cmake on Jenkins?

Clearly, linking doesn't work. As for why it shouldn't be fixed to work just like autotools do, perhaps @iphydf will answer that.

@nurupo
Copy link
Member

nurupo commented Jan 2, 2017

Clearly, linking doesn't work. As for why it shouldn't be fixed to work just like autotools do, perhaps @iphydf will answer that.

It's linking in qTox which doesn't work though. @tux3 has fixed it in qTox/qTox#4038.

I suggest closing this issue as it's not an issue in toxcore but rather in qTox.

@zetok
Copy link
Author

zetok commented Jan 2, 2017

@nurupo You've missed the point that cmake toxcore breaks stuff that works with autotools. This is purely a toxcore issue.

@GrayHatter
Copy link

Works in uTox. This is a qTox issue.

@GrayHatter GrayHatter closed this Jan 2, 2017
@zetok
Copy link
Author

zetok commented Jan 3, 2017

@GrayHatter

Works in uTox. This is a qTox issue.

As I've mentioned above, it's a toxcore issue.

If you're insisting though that it should be closed since it works for some people while it doesn't for other, why not close your own PR (#368)? After all, ToxMe works not just for some people, but for everyone except µTox. Makes a lot more sense to close it.

Reopening. Feel free to close it right after closing your own PR, or if there's a consensus that making ~tox development harder with cmake is must, and easier development with autotools is not allowed.

@zetok zetok reopened this Jan 3, 2017
@iphydf
Copy link
Member

iphydf commented Jan 3, 2017

:lgtm: please rebase. Also, could you create an issue for updating these docs for cmake? We can't fully move to cmake before all important use cases (in particular, cross compilation) are covered.


Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@iphydf iphydf changed the title docs(INSTALL): Bring back autotools instructions Bring back autotools instructions Jan 3, 2017
@iphydf iphydf added this to the v0.1.3 milestone Jan 3, 2017
@zetok
Copy link
Author

zetok commented Jan 4, 2017

Rebased.

@cebe
Copy link
Member

cebe commented Jan 4, 2017

Still would be good to have a description of the issue you face with cmake to be able to solve it.

@iphydf
Copy link
Member

iphydf commented Jan 4, 2017

@nurupo is working on proper cmake instructions. Would you prefer we avoid mentioning it until those instructions are done?

@zetok
Copy link
Author

zetok commented Jan 4, 2017

Would you prefer we avoid mentioning it until those instructions are done?

Documentation of functionality is always a good thing. Cmake usage should be documented, and IMO it shouldn't be delayed until $certain_point if the functionality is already present.

Cmake docs probably should have a remark somewhere that it might not yet be providing the same level of functionality for client devs as autotools.

Regarding making an issue, I'll do it once this PR gets merged (wouldn't make much sense to make it right now since docs haven't been affected yet).

@iphydf iphydf self-assigned this Jan 5, 2017
@iphydf
Copy link
Member

iphydf commented Jan 5, 2017

:lgtm_strong:


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

Reverted, since apparently cmake is not supposed to be used for things
other than testing/development and causes client build failures.

Apparently making it work for clients would require complicating
maintenance, which clearly can't be done.

This reverts commit 48ddb11.
@iphydf iphydf merged commit 69e1b99 into TokTok:master Jan 5, 2017
@zetok zetok deleted the docs-install-autotools branch January 5, 2017 11:04
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.

7 participants