Skip to content

Conversation

@theodelrieu
Copy link

Hi!

I've added basic Conan support for Outcome, there are some issues left:

  • Do we want to build unit tests inconditionally? (gcc 7.2 crashed on my Debian)
  • I'm only packaging the single-header, since QuickCppLib is not installed by ninja install.

I didn't want to change the CMakeLists.txt before having your feedback, let me know if you want to support more than the single-header.

@ned14
Copy link
Owner

ned14 commented Oct 24, 2017

I've added basic Conan support for Outcome, there are some issues left:

Firstly, wow thanks for adding this. It's literally on the list of things I must do before the next peer review, so this is a big help.

Do we want to build unit tests inconditionally? (gcc 7.2 crashed on my Debian)

That's a known problem with the GCC 7 series. Outcome makes it corrupt its memory. GCC 6 has the same problem, but the memory corruption doesn't appear anything like as frequently. clang and MSVC work correctly.

I'm only packaging the single-header, since QuickCppLib is not installed by ninja install.

Yeah, I know, and I'm sorry about that. I need to go finish bashing cmake with a stick regarding correct make install headers installation. It's part implemented, I really ought to finish it.

In the meantime, the source tarball at https://dedi4.nedprod.com/static/files/outcome-v2.0-source-latest.tar.xz is always master branch with all submodules correctly bundled in. Conan could use that.

What do you think? Do Conan's users just want the library, or do they want to see if it works first?

If the tarball is useful, it's trivial for me to have the cronjob upload the tarball to anywhere else too so long as it speaks curl. Do advise on what you think is best.

And once again thanks for this, it's a big help.

@theodelrieu
Copy link
Author

What do you think? Do Conan's users just want the library, or do they want to see if it works first?

I think we should add an option to build unit tests, defaulted to False. Since there's no release yet it would be really tedious for users to build the tests each time.

If the tarball is useful, it's trivial for me to have the cronjob upload the tarball to anywhere else too so long as it speaks curl. Do advise on what you think is best.

I don't believe this is needed, if the build_tests options is set to True, we can simply keep the same build method in the conanfile.py, while still packaging the single header only.

I'll let you know if I'm mistaken, I need to make some changes to the recipe, the tarball might become useful after all.

@theodelrieu
Copy link
Author

I have a question: Do you wish to keep Conan files inside your repo?

This has some implications. In order to provide a "latest" mechanism, one can define a build_policy (i.e. always fetch the latest version).

However, I don't think it makes a lot of sense when the recipe is inside the repo, since you usually don't fetch the source in that case (I did that in the first commit though...).

I believe it would be easier if the recipe was outside, in fact the first version of the conanfile.py was written with this idea in mind (at work we have a conan-recipes repository, that's why I'm not used to put it inside projects).

Plus, providing a latest mechanism with the recipe in-source requires a cronjob of some sort to repackage and upload, which can be annoying to setup.

What do you think about it?

@ned14
Copy link
Owner

ned14 commented Oct 25, 2017

Be aware that the tests require quickcpplib to compile, plus do not run against the single file include. Therefore I'd say running the tests is only practical on the tarball edition.

@ned14
Copy link
Owner

ned14 commented Oct 25, 2017

Regarding the "conan-recipes" idea, would those live in a separate repo on github? If so I'm good with that, my nightly cronjob which decides when to merge develop branch into master can also push an update to a "conan-recipes" repo.

@theodelrieu
Copy link
Author

Be aware that the tests require quickcpplib to compile, plus do not run against the single file include.

We could also completely skip unit tests, since every commit in master is tested.

Regarding the "conan-recipes" idea, would those live in a separate repo on github?

Yes, and you will not have to update it, I'll update the recipe in a few minutes that you should be able to copy/paste in a separate repo.
After that, you could upload the recipe to the central Conan package repository :)

@theodelrieu
Copy link
Author

theodelrieu commented Oct 25, 2017

It should work fine now, the build_policy = "always" line will always retrieve the latest version of single-header/outcome.hpp on the master branch, when users invoke the command conan install.

Note that you have to copy the conanfile.py and the test_package folder to the other repository.

EDIT: You can add the url attribute after that url = the_recipe_repo_url

@ned14
Copy link
Owner

ned14 commented Oct 25, 2017

Very cool, thank you. I'll try to find some time after the kids are in bed tonight to give this a test run with conan. If it works, I will indeed submit it to the central Conan repository. Thank you very much for your work on this.

@theodelrieu
Copy link
Author

No problem, thanks for your efforts on this lib and your proposals ;)

ned14 added a commit that referenced this pull request Oct 25, 2017
@ned14
Copy link
Owner

ned14 commented Oct 25, 2017

Ok, so your pr is in develop branch in the conan directory. I have had quite a few problems:

  1. I tried getting it to error out if the compiler version is too low, but I just get:
ERROR: Outcome/master@demo/testing: Error in build_requirements() method, line 12
        if self.settings.compiler == "clang" and self.settings.compiler.version < "4.0":
        ConanException: 'settings.compiler' doesn't exist
'settings' possible configurations are none

... which is not helpful.

  1. On Windows I get:
ERROR: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\Users\\ned\\.conan\\data\\outcome\\master\\demo\\testing\\build\\5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9'

Couldn't remove folder, might be busy or open
Close any app using it, and retry

... which is also unhelpful.

I have created a conan repo on bintray though, so if we can get the above working, I can push it.

@theodelrieu
Copy link
Author

Yeah errors are sometimes cryptic... The first one is because I removed the settings os, arch, compiler, build_type, they're not useful since we do not build anything.

Also, recipes should not add constraints on the compiler/version, nor add flags like -std=c++14, this should be done by the consumer's build system.

At least that's what I've seen in Github issues/documentation/other Conan recipes.

ned14 added a commit that referenced this pull request Oct 26, 2017
@ned14
Copy link
Owner

ned14 commented Oct 26, 2017

Ok, that hint of yours led to lots more progress. I now get this:

ned@lyta:~/windocs/boostish/outcome/conan$ conan upload Outcome/master@demo/testing --all -r=outcome
Uploading Outcome/master@demo/testing
Uploading conanmanifest.txt
[==================================================] 58B/58B
Uploading conanfile.py
[==================================================] 741B/741B
Uploaded conan recipe 'Outcome/master@demo/testing' to 'outcome': https://api.bintray.com/conan/ned14/Outcome
ERROR: Conanfile has build_policy='always', no packages can be uploaded
  1. I assume that error is benign and can be ignored? Our conanfile.py script always fetches latest from master branch on github anyway.

  2. At https://bintray.com/ned14/Outcome I now see an "Outcome:demo" item. Surely this is not the right form? I surely need some "Outcome" to which other conan packages can say they have build_requirements right?

My thanks to you for putting up with these no doubt silly questions.

@theodelrieu
Copy link
Author

theodelrieu commented Oct 26, 2017

  1. The issue here is that conan upload --all will try to upload the recipe, and every binary package.

To only upload the recipe you have to remove the --all option.

  1. I've never uploaded packages to Bintray, so I may be wrong, but the demo here seems to be the username you gave to the conan upload command.

A conan reference has the following form: project_name/version@user/channel

In this case you might want to use ned14 instead of demo ;)

About the channel, I would keep testing for now, and switch to stable a bit later, once everything is set up correctly on Bintray.

There is no strict rule about the channel name, although stable and testing are quite conventional.

No problem, glad to help!

EDIT: About consumers of your package, they will simply add the reference to their recipe/project: Outcome/master@ned14/stable. Et voilà :)

@ned14
Copy link
Owner

ned14 commented Oct 28, 2017

Ok, can you test for me Outcome/master@ned14/stable and if it works, I'll consider this pull request complete.

@theodelrieu
Copy link
Author

I've tried to download the recipe with conan install with no success, it hangs forever.
It might be a settings problem since I could download the conanfile.py and conanmanifest.txt files with cURL (following the Bintray tutorial).

conan install worked with this package, and the only real difference I've spotted so far is that it contains binary packages, whereas Outcome does not.

I'll upload the recipe on my own server, to see if this is a Bintray-specific problem.

One minor thing, you might want to rename the top-level Outcome to something like conan-repo, that will be handy for future packages you wish to upload, like AFIOv2 ;)

@theodelrieu
Copy link
Author

I've filed an issue on Conan repo, which is unrelated to the Bintray one we're having right now.

I did manage to download the recipe from my own server, so something is certainly up with you Bintray settings.

The aforementioned Conan issue is quite blocking though, since it fails on Windows every time... I should have checked that earlier, sorry about that.

@theodelrieu
Copy link
Author

The issue is fixed, once Conan 0.29 is out, I will test again on Windows.

@ned14
Copy link
Owner

ned14 commented Nov 7, 2017

Ok thanks. Been busy preparing for Meeting C++ this week, will return my attention to this issue upon return.

@theodelrieu
Copy link
Author

I've tried again on Windows, it works (the test_package fails to compile, but that's because of my old Visual Studio).

Closing the PR

@theodelrieu theodelrieu closed this Dec 5, 2017
@theodelrieu theodelrieu deleted the feat/conan branch December 5, 2017 10:07
@Manu343726
Copy link

@theodelrieu where's the recipe maintained? I want to release a package with a stable version (v2.0).

@ned14
Copy link
Owner

ned14 commented Dec 10, 2018

@Manu343726
Copy link

Manu343726 commented Dec 10, 2018

Oh, didn't see the folder, sorry. Could you please release an stable package? That is, one with version = 2.0, that downloads files from the v2.0 release, and doesn't have the build_policy='always'?

Note that changing the recipe that way and uploading the new package doesn't invalidate your previous always-up-to-date package outcome/master.

@ned14
Copy link
Owner

ned14 commented Dec 10, 2018

I have no experience with Conan. Its support was donated. If it isn't working, I can remove it entirely.

@Manu343726
Copy link

The recipe is working, but it's written in a way that it always fetch the latest version from master, which could potentially break user code. Instead, I suggest changing the recipe so it points to the latest stable release (v2.0 iirc). Changing the recipe and releasing a new package tagged with v2.0 will not invalidate the previous distribution pointing to latest master content, both packages will coexist and users will be free to pick whatever version of the package they want.

If you have no experience with conan I will be glad to send a PR with the changes.

@ned14
Copy link
Owner

ned14 commented Dec 10, 2018

Then the recipe is correct. master branch reflects the last commit on develop branch to pass all tests on all platforms. Whilst expected to be mostly stable bar the constant workarounding for breakage of certain compiler versions, master branch won't be written into stone until 2020 after which any API or ABI breakage will be disallowed. Until then, all there is is "v2.1-master".

I appreciate this is annoying, but quite literally most of the changes to Outcome in the past six months have been new compiler workarounds for newly released compilers. There seems no point in choosing a specific SHA until compiler vendors stop breaking Outcome.

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.

3 participants