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

Intake #171

Closed
wants to merge 24 commits into from
Closed

Intake #171

wants to merge 24 commits into from

Conversation

mvictoras
Copy link
Contributor

New pull request based on the changes that we discussed.
There is no intake branch anymore?

@lovell
Copy link
Owner

lovell commented Feb 25, 2015

Hi Victor, the intake branch was the work-in-progress for the 0.9 release. As you've guessed, the master branch is probably the best home for this at the moment as things seem pretty stable with the current release.

Are you able to rebase against lovell:master first then squash your changes into a single commit?

'include_dirs': [
'<!(PKG_CONFIG_PATH="<(PKG_CONFIG_PATH)" pkg-config --cflags vips glib-2.0)',
'<!(node -e "require(\'nan\')")'
'conditions': [
Copy link
Owner

Choose a reason for hiding this comment

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

Is this conditionality required? pkg-config --cflags vips already includes the openslide dependency (when present).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to test that, I think you are right.

@lovell
Copy link
Owner

lovell commented Feb 25, 2015

I'd like to take John's advice and use vips_type_find at runtime to build a map of available features, which could include Openslide support. This should provide a more generic equivalent of the hasOpenslide this PR adds.

The compile-time #ifdef HAS_OPENSLIDE around libvips methods that only exist when it is compiled with Openslide support would still be required with the current minimum libvips version of 7.40.0.

Would you like me to add the generic hasFeature method before you have to handle any rebase conflicts? I'll try to implement it in such as way as to avoid conflicts with your incoming changes.

@mvictoras
Copy link
Contributor Author

No I did not rebase and squash my changes into a single commit, I just created a pull request. I will do it in my next pull request.

Yes please, add a hasFeature method, so I can handle the rebase conflicts.

@mvictoras
Copy link
Contributor Author

I am also working on an install script for openslide, (like this one), do you want this to be incorporated into the preinstall script? In that case, every time you run preinstall you install openslide and then vips. Another option would be being optional as an argument. For example, you run
./preinstall.sh --with-openslide.

Or it can be a completely new script, ex. preinstall-openslide.sh

@lovell
Copy link
Owner

lovell commented Feb 26, 2015

I've started on the "hasFeature" API this morning, which for file-formats will hopefully re-use the existing sharp.format.* enum-like Object employed by toFormat.

I will also add openslide for loading and libgsf for saving deepzoom as dependencies to the existing preinstall.sh script for flavours of Linux where a suitable, pre-compiled version is available via package manager (e.g. >=3.4.0 for openslide).

Once these are done your very useful additions should fit more neatly - sorry this has been a bit messy.

@mvictoras
Copy link
Contributor Author

@lovell, I actually already added openslide support in the preinstall.sh script. You can see it here:
https://github.com/mvictoras/sharp/blob/intake/preinstall.sh

They way it works is as follows:
To install only vips (same behavior as before)
./preinstall.sh

To install vips with openslide support:
./preinstall.sh --with-openslide
Checks for openslide and vips. If openslide not installed, it installs it. If vips is already installed but lacks openslide support, then vips gets recompiled with openslide support.

I have tested the script on all platforms EXCEPT Macports and Amazon cloud, since I don't have any of those around. I am currently working on adding openSuse, since this is the distro we use in our servers.

One last thing: regarding your comment for the pre-compiled version of openslide, remember that the pre-compiled version of vips on all platforms has not been compiled with openslide support, so you will need to compile it anyways.

Hope my comment got there in time, so you don't have to re-write the script!

@mvictoras
Copy link
Contributor Author

Of course, we can make openslide mandatory and get rid of the "--with-openslide" option. That is up to you to decide.

@lovell
Copy link
Owner

lovell commented Feb 26, 2015

Pre-compiled libgsf-1-dev(el) seems to be universally available and I was going to add (lib)openslide-dev(el) only where the package was readily available for a given version of Linux.

I like your idea of an optional --with-openslide to compile openslide from source for the others, so please keep that in the PR, thank you.

Getting openSUSE support in there too would be fantastic, thank you!

@lovell
Copy link
Owner

lovell commented Feb 26, 2015

Commit c7ccf68 should hopefully make things a little easier, albeit with a few conflicts when you rebase.

The new format attribute allows things like:

if (sharp.format.magick.input.buffer) {
  // I can load GIF images from a Buffer
  sharp(gifBuffer)...
}

... which you'll see the tests are using too - much simpler than the semver logic before. You might even be able to remove most if not all of the #ifdef logic from your changes.

You'll see the openslide and dz formats are checked, waiting for your new API to expose them.

Commit 1565522 makes libgsf a dependency, no big deal as it's pre-compiled for all the Linux varieties and homebrew already includes it.

Finally, I need to configure Snap CI to use a modern compiler as it's still using g++ 4.4, which can't handle C++11 properly, hence the failures we're seeing there.

@mvictoras
Copy link
Contributor Author

Great, thank you!
Will be working on this over the weekend, want to have this done by Monday.

@mvictoras
Copy link
Contributor Author

Also have you had the chance to test on Windows too?

@lovell
Copy link
Owner

lovell commented Feb 27, 2015

(Lack of) Windows support will be covered by #19 - it's proving the trickiest OS to get everything working on.

@mvictoras
Copy link
Contributor Author

Great, thanks!

@lovell
Copy link
Owner

lovell commented Mar 12, 2015

Superseded by #146.

@lovell lovell closed this Mar 12, 2015
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.

2 participants