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 uDig #7488

Merged
merged 1 commit into from
Nov 20, 2014
Merged

Add uDig #7488

merged 1 commit into from
Nov 20, 2014

Conversation

vitorgalvao
Copy link
Member

Closes #7484.

@vitorgalvao vitorgalvao mentioned this pull request Nov 20, 2014
vitorgalvao added a commit that referenced this pull request Nov 20, 2014
@vitorgalvao vitorgalvao merged commit 9eff695 into Homebrew:master Nov 20, 2014
@vitorgalvao vitorgalvao deleted the fix-7484 branch November 20, 2014 18:21
@rolandwalker
Copy link
Contributor

This appears to be a suite artifact, rather than an app.

Now that we have completed the DSL transition, we can start enforcing that app artifacts accept .app bundles as values.

@vitorgalvao
Copy link
Member Author

You’re correct; I missed the error in the original PR.

Opening it, though (I’ve downloaded it to make sure), this is actually another good example, same as gridwars. I’ve corrected it as suite for the time being, but we should still iron out how that’s supposed to work. In it’s current state, I do believe suite to be a useless stanza.

@rolandwalker
Copy link
Contributor

In most cases, we link the app bundle directly, even when upstream recommends moving a containing folder into Applications. In some cases, after doing so, the application actually will not run. Thus, suite is required to exist.

As to the Cask at hand, I did not investigate whether it would be better resolved by changing to an app artifact, but it should be one or the other.

@vitorgalvao
Copy link
Member Author

but it should be one or the other

That I agree with, it was my oversight when checking the original submission.

Thus, suite is required to exist.

Why, though? We went along just fine, using just link. Separating link into app and suite brings no tangible benefit; on the contrary. My confusion stems from the fact suite was supposed to work as an alternative to the copy stanza; if we can only use app or suite and they do the same, then we still need a copy stanza. That, or we can never use app in cases where the app bundle comes inside a directory with more files, making it a worse experience for people that use the default behaviour of homebrew-cask (linking).

@rolandwalker
Copy link
Contributor

We are talking about more than one thing, from more than one direction. I do intend to deliver whatever functionality you want with regard to copying; there's only a question of interface.

To completely rewind and clarify history, the first purpose for separating link into specific artifacts was to enable new functionality. When the backend knows that an app is an application, it can eg

  • consult the plist to get the version number
  • manipulate the plist
  • launch the app
  • quit the app

It may not be much apparent (in part because of docs) but we already started using the knowledge from the more-structured Cask. @federicobond did some stuff where we identify the plist which is then used for such features as supress_move_to_applications in the postflight mini-DSL.

When we just had the link artifact, that code would sometimes work, other times not. So far, what suite does is inform the backend "this thingie goes into Applications, but don't confuse it for an app bundle".

Also, in reality, app and suite worked as synonyms for link until three weeks ago (#7065). That was intended to make the transition happen smoothly, which worked, but we also couldn't differentiate on functionality until now. So plenty of things aren't tangible yet, but we have been adding enabling tech.

Our linked conversation above may be just a miscommunication. As I was defining suite for other reasons, I noticed its relation to the idea you mentioned of app-with-supporting-resources. It may turn out that we were wrong – that our two concepts don't fit under the same umbrella. However, I'm totally not worried about that, because

  • we both want the copy functionality
  • we only have to work out interface
  • any interface changes wouldn't require removing elements from the DSL

I do think I misunderstood your recent comment on the roadmap. To reflect what I now think you are working on: in the current repo there are

  • Casks that use suite for style reasons
  • Casks that use suite because app breaks some assumptions in the software about external resources/location (link vistrails app bundle, not parent dir #2513)
  • Casks that use app and the application is self-contained
  • Casks that use app — but the application actually does depend on external resources. These apps are finding their way back to the staging area via reading the symlink and using the resources there.

Your main issue being that the fourth category is not yet being annotated in any way, meaning that future naive copies would break.

I would also note that using the same artifact for the first two categories might be a mistake at some level. Not sure.

@vitorgalvao
Copy link
Member Author

Thank you for the extended explanation on the differences between app and suite: now I get it. Now it makes sense.

Our linked conversation above may be just a miscommunication.

It was. I realised we had different concepts about suite, so I was trying to clarify in my head what the differences in our ideas were, and how that affected copy.

Your main issue being that the fourth category is not yet being annotated in any way, meaning that future naive copies would break.

Precisely. Not only that they would break if left as they are, but that to change how they are now to prevent breakage would lead to a worse interface when linking.

@Homebrew Homebrew locked and limited conversation to collaborators May 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants