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

Add gsl/2.7 #6098

Merged
merged 32 commits into from
Sep 23, 2021
Merged

Conversation

rl-enjoyer
Copy link
Contributor

@rl-enjoyer rl-enjoyer commented Jun 29, 2021

Specify library name and version: gsl/2.7

This is also a good place to share with all of us why you are submitting this PR (specially if it is a new addition to ConanCenter): is it a dependency of other libraries you want to package? Are you the author of the library? Thanks!

close #6097


  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've read the guidelines for contributing.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@CLAassistant
Copy link

CLAassistant commented Jun 29, 2021

CLA assistant check
All committers have signed the CLA.

@rl-enjoyer rl-enjoyer force-pushed the gnu-scientific-library branch from fdefb32 to 234855f Compare June 29, 2021 20:23
@rl-enjoyer
Copy link
Contributor Author

Implements #6097

@rl-enjoyer rl-enjoyer changed the title WIP: Add gsl/2.7 Add gsl/2.7 Jun 29, 2021
@rl-enjoyer rl-enjoyer marked this pull request as ready for review June 29, 2021 21:14
@conan-center-bot

This comment has been minimized.

@Croydon
Copy link
Contributor

Croydon commented Jun 30, 2021

Split out the changes in the .gitignore file to a separate pull request

@rl-enjoyer rl-enjoyer force-pushed the gnu-scientific-library branch from 13289eb to 47d5ec2 Compare June 30, 2021 17:45
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@rl-enjoyer
Copy link
Contributor Author

rl-enjoyer commented Jun 30, 2021

@Croydon , I am confused as to what the error is telling me. Was wondering if you could provide guidance?

@conan-center-bot

This comment has been minimized.

Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@uilianries
Copy link
Member

@NolanC33 Consider rl-enjoyer#1

@rl-enjoyer
Copy link
Contributor Author

Seems like KB-H061 is missing from the error knowledge base markdown?

Seems so.
The error message shows the line number containing the offending code.
You can't use tools.os_info.is_windows in build_requirements.
Replace it with self._settings_build.os == "Windows" and create a _settings_build helper method.

You can find an example in the swig recipe.

Awesome, thanks. Out of curiosity, what's the rationale behind that requirement?

@madebr
Copy link
Contributor

madebr commented Sep 21, 2021

This was the trigger: #6348 (comment)

@rl-enjoyer
Copy link
Contributor Author

will remove conan. from lines 131 and 132

@conan-center-bot

This comment has been minimized.

@rl-enjoyer
Copy link
Contributor Author

Not too sure about the other error:

m4/1.4.18: WARN: Lib folder doesn't exist, can't collect libraries: C:\J\w\BuildSingleReference@9\.conan\data\m4\1.4.18\_\_\package\0a420ff5c47119e668867cdb51baff0eca1fdb68\lib
autoconf/2.71: WARN: Lib folder doesn't exist, can't collect libraries: C:\J\w\BuildSingleReference@9\.conan\data\autoconf\2.71\_\_\package\5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9\lib

@madebr
Copy link
Contributor

madebr commented Sep 21, 2021

Those are warnings.
They are emitted by a loose check of the conan-center hook.
It does a call to tools.collect_libs(ConanFile), which warns if the directories in self.cpp_info.libdirs do not exist.
The autoconf and m4 package do not ship a library, only scripts/executables (so there is no lib subfolder)

To eliminate these types of warnings, I have added self.cpp_info.libdirs = [] in my recent m4 recipe.

@rl-enjoyer
Copy link
Contributor Author

rl-enjoyer commented Sep 21, 2021

Ah ok. They do seem to sink the build though. At least I can't find anything else in the log that could be it. Also interesting how the warnings arise on only one configuration for both 2.6 & 2.7.

@madebr
Copy link
Contributor

madebr commented Sep 21, 2021

Those are warnings, the error was due to the typo bug in the package method.
It looks like the build is succeeding now.
There are only Visual Studio jobs in the build queue now.

@rl-enjoyer
Copy link
Contributor Author

Ok cool. If you look at https://c3i.jfrog.io/c3i/misc/logs/pr/6098/17-configs/windows-visual_studio/gsl/2.7//995e0f0b86a651012a3bfca00d60f35ae037db5e-build.txt you see the complaint about the libs. And not the complaint about the rename typo. (from https://c3i.jfrog.io/c3i/misc/summary.html?json=https://c3i.jfrog.io/c3i/misc/logs/pr/6098/17-configs/windows-visual_studio/gsl/2.7//summary.json)

Now if you look at https://c3i.jfrog.io/c3i/misc/logs/pr/6098/17-configs/windows-visual_studio/gsl/2.7//f42ebec2d0ee7d5c41713cd8eee15cf988b0d202-build.txt it complains about the rename typo and not the libs. (also from https://c3i.jfrog.io/c3i/misc/summary.html?json=https://c3i.jfrog.io/c3i/misc/logs/pr/6098/17-configs/windows-visual_studio/gsl/2.7//summary.json)

I suppose we'll see once the current jobs are done, but if f42ebec2d0ee7d5c41713cd8eee15cf988b0d202 failed because of the typo, would we expect to see that in the 995e0f0b86a651012a3bfca00d60f35ae037db5elog as well?

@madebr
Copy link
Contributor

madebr commented Sep 21, 2021

The warning about the libdirs is shown after executing the package_info method (because it verifies self.cpp_info.libdirs).
So that warning is always shown at the very end.

The rename typo bug happens in package, way before package_info, so the warning is not there.

@rl-enjoyer
Copy link
Contributor Author

The warning about the libdirs is shown after executing the package_info method (because it verifies self.cpp_info.libdirs).
So that warning is always shown at the very end.

The rename typo bug happens in package, way before package_info, so the warning is not there.

Ahh ok, thanks

@conan-center-bot

This comment has been minimized.

madebr
madebr previously approved these changes Sep 21, 2021
@rl-enjoyer
Copy link
Contributor Author

Sorry about that. Didn't mean to dismiss your review. Could you repost it? @madebr

@conan-center-bot
Copy link
Collaborator

All green in build 20 (3e11dbf9fe3a4dbbbadfab7f3fd01195b338408a):

  • gsl/2.7@:
    All packages built successfully! (All logs)

  • gsl/2.6@:
    All packages built successfully! (All logs)

@rl-enjoyer rl-enjoyer marked this pull request as ready for review September 22, 2021 18:45
@rl-enjoyer
Copy link
Contributor Author

Do I need to rebase this off master?

@madebr
Copy link
Contributor

madebr commented Sep 22, 2021

That's not needed.
The bot will take care of that.
Since you're contributing a complete new recipe, there's 0% change on merge conflicts.

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

LGTM

@conan-center-bot conan-center-bot merged commit 6bcd837 into conan-io:master Sep 23, 2021
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.

[request] gsl/2.7
8 participants