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.

Sign up for GitHub

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

Jump to bottom

Revert "Merge pull request #20 from Tobias-Fischer/patch-1" #23

Merged
merged 5 commits into from
Jan 5, 2022
Merged

Revert "Merge pull request #20 from Tobias-Fischer/patch-1" #23

merged 5 commits into from
Jan 5, 2022

Conversation

Tobias-Fischer
Copy link
Contributor

@Tobias-Fischer Tobias-Fischer commented Dec 16, 2021 •
edited
Loading

This reverts commit 76ec8ee, reversing
changes made to f2fda65.

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

Fix #21

See #22 - it seems like the better way (at least for now) is to go back to the autotools build.

I am unsure about:

  • whether to rerender and bump the build number (if so, bump by 2 as the broken cmake build was a bump by 1?)
  • whether to merge as is, and if so whether to add [ci skip] [skip ci] [cf admin skip] ***NO_CI***?

@conda-forge/yaml - please feel free to edit the PR as needed or let me know which option you prefer.

Sorry again for messing up the cmake build!

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

jakirkham
jakirkham reviewed Dec 16, 2021
View reviewed changes
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Tobias! 😄

recipe/meta.yaml
@@ -37,10 +37,6 @@ test:
- if not exist "%LIBRARY_LIB%\\yaml.lib" exit 1 # [win]
- if not exist "%LIBRARY_BIN%\\yaml.dll" exit 1 # [win]

# Check cmake and pkg-config scripts
- test -f "${PREFIX}/cmake/yamlConfig.cmake" # [not win]
- test -f "${PREFIX}/lib/pkgconfig/yaml-0.1.pc" # [not win]
Copy link
Member

Choose a reason for hiding this comment

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

Should we keep the pkgconfig test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy either way - let me know what you prefer.

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep the test

@Tobias-Fischer
Copy link
Contributor Author

Any more input on this PR (and in particular to my questions above)?

@jakirkham
Copy link
Member

@beckermr @ocefpaf, any thoughts here? 🙂

@beckermr
Copy link
Member

beckermr commented Jan 4, 2022

Let's bump by two and merge once the build passes.

jakirkham
jakirkham reviewed Jan 4, 2022
View reviewed changes
recipe/meta.yaml Outdated Show resolved Hide resolved
recipe/meta.yaml
@@ -37,10 +37,6 @@ test:
- if not exist "%LIBRARY_LIB%\\yaml.lib" exit 1 # [win]
- if not exist "%LIBRARY_BIN%\\yaml.dll" exit 1 # [win]

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Check cmake and pkg-config scripts
- test -f "${PREFIX}/cmake/yamlConfig.cmake" # [not win]
- test -f "${PREFIX}/lib/pkgconfig/yaml-0.1.pc" # [not win]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This also added the cmake test, which won't pass as it's not a cmake build anymore.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok. Sorry let's drop that then. Thanks for catching this 🙂

@jakirkham
Apply suggestions from code review
05c70f8 
* Bump build number to 2
* Readd pkg-config tests
@jakirkham jakirkham added the automerge Merge the PR when CI passes label Jan 4, 2022
@github-actions github-actions bot removed the automerge Merge the PR when CI passes label Jan 4, 2022
@github-actions
Copy link

github-actions bot commented Jan 4, 2022

Hi! This is the friendly conda-forge automerge bot!

Commits were made to this PR after the automerge label was added. For security reasons, I have disabled automerge by removing the automerge label. Please add the automerge label again (or ask a maintainer to do so) if you'd like to enable automerge again!

@Tobias-Fischer
Copy link
Contributor Author

@conda-forge-admin, please rerender

@Tobias-Fischer
Copy link
Contributor Author

Fix #22

@jakirkham
Copy link
Member

Thanks Tobias! 😄

@jakirkham jakirkham merged commit 7891f97 into conda-forge:master Jan 5, 2022
@Tobias-Fischer Tobias-Fischer mentioned this pull request Feb 8, 2022
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakirkham jakirkham jakirkham left review comments

@mingwandroid mingwandroid Awaiting requested review from mingwandroid

@msarahan msarahan Awaiting requested review from msarahan

@ocefpaf ocefpaf Awaiting requested review from ocefpaf

@pelson pelson Awaiting requested review from pelson

@seanyen seanyen Awaiting requested review from seanyen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New build of yaml has wrong library name
4 participants
@Tobias-Fischer @conda-forge-linter @jakirkham @beckermr