Skip to content

dub run <package> should search the registry on failure #1428

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

Merged
merged 1 commit into from
Feb 19, 2019

Conversation

wilzbach
Copy link
Member

(Reopened #1082 as I accidentally closed it during the push)

This has been pending in the queue for quite a while. Anything blocking this?

@dlang-bot
Copy link
Collaborator

Thanks for your pull request, @wilzbach!

@wilzbach
Copy link
Member Author

wilzbach commented Apr 9, 2018

BTW now that we show instructions on how to build a dlang PR locally:

image

(example: dlang/dmd#8155)

This would be even more interesting imho.
(Of course, I'm aware we would have to wait a bit until we can remove dub fetch from the instructions, but we have to start somewhere and this has been in the queue for more than a year already)

@wilzbach
Copy link
Member Author

Ping on this. I still think this would be nice to have and there haven't been any objections.
Maybe we can finally get this in for the next release?

Copy link
Member

@AntonMeep AntonMeep left a comment

Choose a reason for hiding this comment

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

It's a nice feature. I'd like to use this in scripts and such, but it won't be possible since it waits for user input. Can we have -y command line option to assume "Yes" as answer to the prompt?

@wilzbach
Copy link
Member Author

Can we have -y command line option to assume "Yes" as answer to the prompt?

Good idea. Added.

writefln("Description: %s", p.description);
writefln("Version: %s", p.version_);

bool answer = m_yes ? true : input("Do you want to fetch %s?".format(package_name));
Copy link
Member

Choose a reason for hiding this comment

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

I like the change but I think we should move the m_yes/input method into a more general scope where all methods could read a boolean input

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there anything else that currently uses this?
We could have easily done this after this PR has been merged and this simple improvement has been in the queue now for more than 1 1/2 years (see the original PR).

@andre2007
Copy link
Contributor

As dub run now implicit calls fetch, I wonder whether dub run should also have an argument "version".
If I run a CI build pipeline it must always have the same result. Therefore i would like to specify the version of the tool I ran with dub run.

On the other hand one can say that is a special case for which I can use dub fetch && dub run.

@PetarKirov
Copy link
Member

Ping @wilzbach what's the status of this? I'm very much in favor.

@wilzbach
Copy link
Member Author

This has been ready since three months except for the nitpick. We could always do that one later though.

@wilzbach
Copy link
Member Author

Rebased.

@thewilsonator
Copy link
Contributor

This appears to break a bunch of shell script tests.

@andre2007
Copy link
Contributor

andre2007 commented Dec 5, 2018

E.g. test issue1158-stdin-for-single-files.sh fails because package_name (free_args[0]) contains value
C:\Users\User\AppData\Local\Temp\app_dcb374c2_7b1e_4536_9027_d1d10347846f.d

Maybe --single does not play well with your new logic. Therefore the tests fails which uses dub run --single file.d

@andre2007
Copy link
Contributor

I just realized, with this feature the custom init idea from @MartinNowak (#600) becomes a low hanging fruit. Just a matter of rewriting the args[] array.

@wilzbach wilzbach force-pushed the fix-877 branch 2 times, most recently from 95120bc to aeb7e0b Compare January 31, 2019 07:26
@wilzbach
Copy link
Member Author

All green now.

return 2;

auto p = search.front.front;
writefln("%s wasn't found locally, but it's available online:", package_name);
Copy link
Member

Choose a reason for hiding this comment

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

Don't use std.stdio directly, as this circumvents dub's logging infrastructure. Use logInfo instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm in dub init std.stdio.writef is used too.

auto pack = dub.packageManager.getFirstPackage(package_name);

if (!pack) {
bool input(string caption, bool default_value = true) {
Copy link
Member

Choose a reason for hiding this comment

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

There is at least one more local definition of input (e.g. for dub init) in this file. I'd like if we could generalize this into a reusable free function + as @WebFreak001 mentioned the y/N handling code.
Not a strong requirement from my side, just nice to do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm the dub init input is quite different to this input as (1) a default value is allowed and (2) multiple values are allowed for a single value (all yes, y, 1, YeS, Y, ...) map to 1.

Though I'm not against unifying them by any means if someone wants to try it after this addition.

if (free_args.length < 1 || m_single)
return super.execute(dub, free_args, app_args);

const package_parts = free_args[0].split(":");
Copy link
Member Author

Choose a reason for hiding this comment

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

This could use splitPackageName from #1643

@wilzbach
Copy link
Member Author

Rebased, used splitPackageName from #1643 and should hopefully be good 2 go now (:

We missed 2.085.0 by a day, but well at least we can get it in the next release.

@wilzbach
Copy link
Member Author

Ping @thewilsonator

@dlang-bot dlang-bot merged commit e52567a into dlang:master Feb 19, 2019
@MartinNowak
Copy link
Member

We already have a --non-interactive option for other commands, would be better to consistently stick with it.

@MartinNowak
Copy link
Member

Well --yes and --non-interactive are semantically differrent.

MartinNowak added a commit to MartinNowak/dub that referenced this pull request Apr 28, 2019
- to match .editoconfig and the rest of the code
wilzbach added a commit that referenced this pull request Apr 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants