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

Bump imagemagick and make #1192

Merged
merged 2 commits into from Jan 5, 2017
Merged

Bump imagemagick and make #1192

merged 2 commits into from Jan 5, 2017

Conversation

rasa
Copy link
Member

@rasa rasa commented Jan 4, 2017

No description provided.

@@ -19,7 +22,7 @@
"conjure.exe",
"convert.exe",
"dcraw.exe",
"ffmpeg.exe",
["ffmpeg.exe", "iffmpeg"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

For what is this alias?

Copy link
Member Author

@rasa rasa Jan 5, 2017

Choose a reason for hiding this comment

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

The ffmpeg executable shipped with Imagemagick is rather dated. Aliasing the shim allows us to install the ffmpeg package alongside Imagemagick. Is there a better way?

Copy link
Member

Choose a reason for hiding this comment

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

@rrelmy how does that sound to you? I will merge this since the current version is 404'ing, but please let us know or submit a pull request if there's a better way.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm happy to undo it, if it's an issue. We could solve this in one of three ways:

  1. Add a "priority" flag to each app (or each exe), and assign a higher priority to ffmpeg. install/uninstall then needs to search all installed apps to see if any app supplies an exe. Ugh.
  2. When a new app installs/updates over a shim, ask the user which one s/he wants. Breaks the ffmpeg command, if the user selects the imagick version, but later uninstalls it. Yuk.
  3. Add ffmpeg's path to PATH before the shims path. Yuk, but doesn't break on uninstall.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's fine for me, I just was curious.

A fourth option would be removing this alias completely

Copy link
Member Author

@rasa rasa Jan 6, 2017

Choose a reason for hiding this comment

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

Another option would be to remove ffmpeg from imagick's bin list and add a "depends": ["ffmpeg"]. That's probably the best solution. Agreed?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, agreed 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

I did that change in #1197 ;-)

@lukesampson lukesampson merged commit 42380d1 into ScoopInstaller:master Jan 5, 2017
rrelmy added a commit to rrelmy/scoop that referenced this pull request Jan 7, 2017
lukesampson pushed a commit that referenced this pull request Jan 7, 2017
* fix autoupdate hash regression

* update nodejs-lts 6.9.4

* add autoupdate.note

* update mercurial 4.0.2 and add autoupdate

* fix autoupdate if no arch specific options are set

* update syncany-cli 0.4.7 and add autoupdate

* update youtube-dl 2017.01.05 and add autoupdate

* update git-lfs 1.5.4 and add autoupdate

* fix far version number

* update imagemagick 7.0.4-3

* replace imagemagick ffmpeg with dependency (as discussed in #1192)

* update modd 0.4 and add autoupdate

* whitelist chocolatey urls from HEAD check

* update scriptcs 0.16.1 and add autoupdate

* update rg 0.3.2 and add autoupdate

* update kotlin 1.0.6 and add autoupdate

* update xz 5.2.3 and add autoupdate

* update openjdk 1.8.0.111-3 and add autoupdate

* update git-up 1.4.1 and add autoupdate

* update ninja 1.7.2 and add autoupdate
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.

None yet

3 participants