Skip download of "vendor" if not necessary

[giuseppe]

No description provided.

[runcom]

I believe vndr itself should do this

[runcom]

@LK4D4 any idea?

[giuseppe]

@runcom I had a quick look at vndr and to get it fixed there, but it turned easier to fix it here (at least for now as quick solution), rm -rf vendor && redownload it for each make test is a bit expensive :(

I can move the second patch to a different PR though if you prefer to get an opinion from @LK4D4 first

[runcom]

@mtrmac @erikh wdyt?

[giuseppe]

second patch moved to: #243

[mtrmac]

How are these rules supposed to work? AFAICS all this does is that it causes the deps.timestamp rule to trigger, which just touches the timestamp file; under no circumstances that would cause deps to be re-run AFAICS. Same for the all dependency.

It does seem that something like the vendor:… vendor.conf; touch vendor rule would be useful to ensure vndr is run only when necessary, but it’s not clear to me how all the pieces in this commit are supposed to fit together.

[giuseppe]

sorry, I screwed it in an attempt to cleanup the patch. test: deps.timestamp should look like test: vendor.

[erikh]

I don't think that's going to work; IIRC make won't trigger on directory updates.

[erikh]

If I remember my make right, touch vendor doesn't really accomplish much.

Also unless I'm missing something, I think there should be a "force a download" target of some sort. It doesn't look like there's something that does that for us here. Maybe the clean target should be adjusted too.

[giuseppe]

@erikh a 'make clean && make deps' will do that. Are there cases where we really want a 'force a re-download of everything'?

[erikh]

@giuseppe if make clean is not updating the timestamp file, won't it silently ignore the vendor download in the current patch form?

[erikh]

Oh I missed your modification to the clean target; sorry!

[mtrmac]

This is better, but still confusing.

• The deps.timestamp and deps targets apparently have absolutely nothing in common.
• Neither of them really handles fetching dependent packages; they handle fetching tools.
• The vndr tool is treated differently from the others.

Perhaps this structure would make sense (?)

# Fetch all tools needed for compilation. The timestamp is an internal implementation detail.
tools: tools.timestamp
tools.timestamp:
    # (go get …) from deps.timestamp and vndr

# Update dependencies in the vendor subdirectory based on vendor.conf
vendor: tools vendor.conf
    @vndr
    @touch vendor

# Remove the vndr rule, subsumed by tools{.timestamp}

(+ relevant deps updates)

[giuseppe]

pushed a new version ⬆️

[LK4D4]

vndr recreates vendor each time and I think it's safer that way and better to fix per repo :)

[giuseppe]

That still looks like a big waste, even to fix per repo. Nothing should happen if the available version is the one the user asked for

[LK4D4]

vndr should be rare. It wasn't really designed for not-in-git vendor.
Without redownloading it will be hard to prevent occasional by-hands changes in vendor IMO. I have a strong opinion that vndr should be run on CI only if vendor.conf or vendor changed and those changes way easier to track with git than in vndr.
However, as I said before I never worked with repos without vendor, so my experience might be really different.
Thanks!

[giuseppe]

I added a new patch to replace vndr with trash (discussion here: LK4D4/vndr#27)

It makes faster also the case when vendor.conf is changed.

[mtrmac]

vndr recreates vendor each time and I think it's safer that way and better to fix per repo :)

@LK4D4 I’m sorry; what does that mean / imply for this project, and what does “fix per repo” mean?

[mtrmac]

I added a new patch to replace vndr with trash (discussion here: LK4D4/vndr#27)

It makes faster also the case when vendor.conf is changed.

If I understand correctly, this PR even before the move to trash will ensure that we don’t run vndr at all during the normal edit/compile/test cycle, so we don’t care that much how long it takes.

Is the benefit that after the user edits vendor.conf, the tool run is faster? Or is there something else more important?

[giuseppe]

@mtrmac the benefit is that once you edit vendor.conf, or for any reason you run vndr, you won't need to pull again all the dependencies. I don't think that even make clean should be so aggressive.

[mtrmac]

I will admit to several times a month editing a copy of the code in vendor/* for a quick experiment. It is definitely valuable to have some command to refresh the state to the truly expected versions. (Not arguing that it needs to be the default behavior.)

I guess I’d slightly prefer merging the immediate productivity fix of not re-running a vendoring tool (any vendoring tool) on every test run, and splitting the tool change to a separate PR, over blocking the productivity fix on a tool choice discussion… but then, *shrug*, I don’t know much about the available choices, I don’t care all that much, and I would just defer to @runcom on the tool choice anyway…

[giuseppe]

@mtrmac sure, moved the second patch here: #244 and dropped from the current PR

[mtrmac]

👍 Thanks!

[runcom]

lgtm