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 jpeg #355

Closed
wants to merge 13 commits into from
Closed

add jpeg #355

wants to merge 13 commits into from

Conversation

gillins
Copy link
Contributor

@gillins gillins commented Apr 14, 2016

Unix build same as conda-recipes.
Had to make some changes to the Windows build to make a DLL - this not supported by provided makefile so use simple cmake script instead that is provided as a patch with this recipe.

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly conda-forge-admin automated user.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/jpeg) and found some lint.

Here's what I've got...

For recipes/jpeg:

  • The recipe could do with some maintainers listed in the "extra/recipe-maintainers" section.
  • The recipe must have some tests.

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly conda-forge-admin automated user.

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

- python # [win]
- cmake # [win]

test:
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned in the cross post, I don't think you need Python for building, but for testing - and you probably need to add a Jinja template to make sure the correct Python is chosen. This should fix the Windows errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I think I've got it working under Windows. Which is the Jinja template? appveyor seems to be running all the various versions of Python.

Copy link
Contributor

Choose a reason for hiding this comment

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

The Jinja template is here. That should get the builds working. That will fix the fact that only Python 2.7 builds seems to work.

@patricksnape
Copy link
Contributor

@msarahan Obviously Continuum already ship libjpeg - how are the Windows builds currently handled? Do you also use a custom CMake?

@gillins
Copy link
Contributor Author

gillins commented Apr 14, 2016

Continuum seem to use the makefile.vc that is supplied. However, this builds a static lib that can only be used by executables but not DLLs (ie GDAL). This workaround was the best I could come up with.

@patricksnape
Copy link
Contributor

@gillins You are totally right, that is very confusing - because packages seem to be able to rely on jpeg and compile yet there is no DLL being created. And I've never set up a package to explicitly statically link jpeg so I'm stumped there. Great work on creating a working CMake for Windows then! I wonder if we need to test that it is exporting the symbols correctly somehow.

@patricksnape patricksnape mentioned this pull request Apr 14, 2016
@patricksnape
Copy link
Contributor

That failure is because of 503 errors :( Need to restart the build!

@gillins
Copy link
Contributor Author

gillins commented Apr 15, 2016

Well the Jinja thing seemed to have fixed it anyway since the Python 3.4 and 3.5 tests passed. Thanks!

Is restarting the build something I can do?

regarding symbols being exported under Windows: well the fact that the utility programs link seems a good test, but I could write some Python to test a list of function names are in the DLL using ctypes or win32.LoadLibrary if you think it is necessary.

@msarahan
Copy link
Member

Sorry guys, I have been sick the past couple of days. Confirming that the Continuum recipe only uses makefile.vc - so what you have here should be an improvement. Wherever possible, it would be nice to provide both static and dynamic libraries, so anyone building with our stuff has the option.

@gillins
Copy link
Contributor Author

gillins commented Apr 15, 2016

Have added static libs for Unix and Windows builds. For Windows the dynamic libs are libjpeg.dll/.lib (the .lib is the import library) and the static lib is jpeg.lib. Not sure if there is a naming convention we should be following here.

Is it usual Conda convention to provide both static and dynamic versions? At one stage Ryan Grout seemed to be removing all the static builds.

AppVeyor seems happy now BTW.

@jakirkham
Copy link
Member

Is it usual Conda convention to provide both static and dynamic versions?

It is a conda-forge convention if that is what you mean, yes. We try to catch this and fix it when we can. If there are ones you know that are missing static libraries, feel free to raise an issue on the respective feedstock or note this in the PR to staged-recipes if it hasn't been released yet.

@@ -0,0 +1,6 @@

Copy link
Member

@jakirkham jakirkham Apr 15, 2016

Choose a reason for hiding this comment

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

Please add #!/bin/bash.

Copy link
Member

Choose a reason for hiding this comment

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

Done.

@ocefpaf ocefpaf mentioned this pull request Apr 15, 2016
@jakirkham
Copy link
Member

This looks pretty nice to me. Thoughts from others?

We might want to squash the binary commits (pictures and such) so we don't have that stuff lingering in the repo's history.

@ocefpaf
Copy link
Member

ocefpaf commented Apr 15, 2016

This looks pretty nice to me. Thoughts from others?

👍

We might want to squash the binary commits (pictures and such) so we don't have that stuff lingering in the repo's history.

@gillins is in Australia, so I don't think anything will be done here in our time-zone. Maybe we could enable squash-your-commits 😉

@jakirkham
Copy link
Member

Ah, thanks for TZ info. Yeah, was thinking about doing it manually, but that is a good point.

@jakirkham
Copy link
Member

Oh, squash merge is not enabled here. 😮

@jakirkham
Copy link
Member

I'll do it the old fashion way.

@jakirkham
Copy link
Member

Squashed and added in this PR ( #369 ).

@ocefpaf ocefpaf closed this in #369 Apr 15, 2016
@ocefpaf
Copy link
Member

ocefpaf commented Apr 15, 2016

Thanks @gillins for the recipe and @jakirkham for squashing it.

@gillins
Copy link
Contributor Author

gillins commented Apr 16, 2016

Yeah seems my TZ isn't very sociable... Thanks everyone for all the help getting this merged

@jakirkham
Copy link
Member

No worries. It was easy enough to bring home. Thanks again @gillins.

@ocefpaf
Copy link
Member

ocefpaf commented Apr 16, 2016

Yeah seems my TZ isn't very sociable... Thanks everyone for all the help getting this merged

Actually, for GitHub interactions, I do like that the PRs sit for a few hours 😉

However, it would be nice to get you in one of our Google Hangouts one day.

(See conda-forge/conda-forge.github.io#89 for the last one.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants