Page MenuHomeFreeBSD

Add Gemfile-check to stage-qa
ClosedPublic

Authored by tz on Aug 4 2017, 2:02 PM.

Details

Summary

Aloha,

inspired by lifanov and his work in PR 220605 to add a check for .gemspec of rubygems i tried myself with Gemfile.

Background is, that checking the actual Gemfile of non rubygem-* ports is often very time-consuming. When building Gitlab, Redmine or others, everything is fine. But when executing they fail - because the Gemfile is not satisfied.

Its WIP and my first try for an stage-qa script, so every comment is appreciated. It adds a stage-qa stage for every non rubygem- port. When executed i (intent) to scan for Gemfiles and checking every file with bundle check. If bundle fails, the stage-qa fails.

It worked for simple test. If no Gemfile was present the test was skipped. If it is, bundle is executed. When removing a needed dependency it is found. Its also found when the dependency is indirect (not in Gemfile itself, but a dependency of an dependency listed there).

But it don't work for net-im/mikutter for example and i don't know why.

So any feedback would be fine!

Greetings,
Torsten

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

#StylePolice: The indentation is wrong, we use tabs here, not two spaces for indentation :-)

lifanov requested changes to this revision.Aug 4 2017, 2:17 PM
lifanov added inline comments.
Mk/Scripts/qa.sh
833 ↗(On Diff #31575)

You also need to gate it on something like USE_RUBY.

836 ↗(On Diff #31575)

Can you pick a different variable name please? It's confusing because you are looking for files named literally Gemfile.

842 ↗(On Diff #31575)

Exit if "bundle" is not available or pick something else to use.

847 ↗(On Diff #31575)

Add a "warn" or "err" here. This needs to include a message to the porter about what's wrong.

This revision now requires changes to proceed.Aug 4 2017, 2:17 PM

(Side note, could you use devel/arcanist, or at least generate a diff with full context like it does, with svn diff -x -U9999 or git diff -U9999. I wanted to look at something a bit earlier in the file, and could not do so here because of missing context.)

Mk/Scripts/qa.sh
837 ↗(On Diff #31575)

You forgot the case:

# No results presents a blank line from heredoc.
[ -z "${Gemfile}" ] && continue
839 ↗(On Diff #31575)

This should be [ ! -f

842 ↗(On Diff #31575)

test that bundle exists?

844–848 ↗(On Diff #31575)

Never test $?. Instead, use if:

if ! bundle ...; then
851 ↗(On Diff #31575)

Shouldn't this be in WRKSRC instead ?

Mk/Scripts/qa.sh
851 ↗(On Diff #31575)

I think STAGEDIR is correct. Gemfile gets installed.

Mk/Scripts/qa.sh
836 ↗(On Diff #31575)

Change to GemfilePath

837 ↗(On Diff #31575)

Added.

839 ↗(On Diff #31575)

Changed.

847 ↗(On Diff #31575)

bundle creates a very good error message - isn't this enough?

851 ↗(On Diff #31575)

I took this part from lifanovs patch. I did find STAGEDIR convenient since its a stage-qa. Also a short try says it does not work with WRKSRC.

tz edited edge metadata.
Mk/Scripts/qa.sh
847 ↗(On Diff #31575)

Hmm, I'm not sure. We do want messages to be uniform for stage-qa targets, but if bundle error message is good, it's a judgement call.

I'm sorry, i mixed up my comments and the diff-update. I really need to get more used to it.

Open points:

  • only test when USE_RUBY = yes
  • only test if bundle exists
  • do not use $?
  • maybe make error message better
Mk/Scripts/qa.sh
847 ↗(On Diff #31575)

bundle prints what gem is missing or which version is not satisfied. It also prints out the load order and dependency chain of the gems. This is very helpful but far away from a uniform stage-qa message.

These two could have different solutions:

  • only test when USE_RUBY = yes

You can export something from bsd.ruby.mk and check for that. This is an obvious one, but you can pick something else if you think it's better.

  • only test if bundle exists

You can have some sort of a simpler fallback or just limit the scope to ports using bundle.

Mk/Scripts/qa.sh
847 ↗(On Diff #31575)

If you think it's good, it might be good. Can you paste a sample output into the "Summary" field above please?

I fixed the usage of the variables, which seems to work just out of curiosity.
I also added an error message for WIP reasons. Currently the check does not work in any circumstance and i currently do not understand why. This needs some more investigation.

Mk/Scripts/qa.sh
847 ↗(On Diff #31575)

cd /usr/ports/www/gitlab && make patch && bundle check --gemfile work/gitlabhq-9.1.9/Gemfile
Resolving dependencies...
The following gems are missing

  • rake (10.5.0)
  • RedCloth (4.3.2)
  • ace-rails-ap (4.1.2)
  • i18n (0.8.1)
  • minitest (5.7.0)
  • tzinfo (1.2.2)
  • activesupport (4.2.8)
  • mini_portile2 (2.1.0)
  • nokogiri (1.6.8.1)

[..]

It seems some Gemfiles loading other Gemfiles and this will fail bundle. No idea how to handle this at the moment.

Mk/Scripts/qa.sh
836 ↗(On Diff #31584)

could you use a lower case variable, possibly shorter, like "f" for file ?

839 ↗(On Diff #31584)

You need to quote the variable.

842 ↗(On Diff #31584)

Quote variable.

845 ↗(On Diff #31584)

Still not testing that bundle exists before calling it.

tz marked 7 inline comments as done.Aug 7 2017, 8:30 AM
tz added inline comments.
Mk/Scripts/qa.sh
836 ↗(On Diff #31584)

Yes, of course. But i wonder why?

I have a hard time to read the other stages because of their cryptic writing and the massive absence of documentation. I think a good named variable makes life much easier. And that a shell script does not have the same restrictions like microcode or other embedded stuff. Or is it just to be consistent to the rest? Or something else?

tz marked 2 inline comments as done.Aug 7 2017, 8:30 AM
  • use guards instead of too many ifs
  • add USE_RUBY check
  • add check for existing bundle before using
  • change var-name from GemfilePath to f
  • quote variable in if-conditions
tz marked 14 inline comments as done.Aug 7 2017, 9:45 AM
  • Switch from error to warning in Output
  • remove return value of 1 when finding error

I decided to relax the check much more. While various tests i found:

  • bundle is not able to always check the gemfile out of various/complex reasons
  • often there are gemfiles in a project which are not executed (QA for example) and which could not satisfied because the needed ports are missing.

Therefore i just only print a message so the maintainer / committer can decide if a further check is needed.

Mk/Scripts/qa.sh
836 ↗(On Diff #31584)

lowercase because we only use uppercase variables for things that come through the environment like make(1) variables that are passed through. Also we do not use camelcase.

As for shorter, I find it makes it harder to read with long variable names, especially as there is only one variable here.

843 ↗(On Diff #31694)
if ! type -p bundle; then
bundle is not able to always check the gemfile out of various/complex reasons
often there are gemfiles in a project which are not executed (QA for example) and which could not satisfied because the needed ports are missing.

In the "nice to have" pile: it would be nice if a port maintainer was able to provide a list of Gemfile files for which to skip the test.

I think this is clean enough to move forward. Can you request an exp-run please?

Change check for bundle according to mat's suggestion

Mk/Scripts/qa.sh
843 ↗(On Diff #31694)

Antonie reports in the PR:

The check looks wrong, when bundle is not installed:

> Running Q/A tests (stage-qa)

-p: not found
bundle: not found
Notice: Please install sysutils/rubygem-bundler for additional Gemfile-checks

I'm unfamiliar with this check. Any suggestion how to improve?

Mk/Scripts/qa.sh
843 ↗(On Diff #31694)

Try "command -v bundle" instead.

I fixed the "type" problem. When redirecting the output it works fine :)

tz marked an inline comment as done.Jan 19 2018, 2:27 PM
tz added inline comments.
Mk/Scripts/qa.sh
843 ↗(On Diff #31694)

It works as wanted when the error output is redirected.

Looks good.

Mk/Scripts/qa.sh
862 ↗(On Diff #38205)

indentation problem here.

tz marked an inline comment as done.

Fix indentation problem :)

tz marked an inline comment as done.Jan 22 2018, 11:05 AM

I think its ready for an exp-run. What do you think?

Mk/Scripts/qa.sh
862 ↗(On Diff #38205)

Now fixed :)

In D11865#294018, @tz wrote:

I think its ready for an exp-run. What do you think?

I do not think an exp-run would show anything. Unless it runs its bulk with -t, which would probably fail horribly, and early, not showing much.

tz marked an inline comment as done.Jan 22 2018, 2:56 PM
In D11865#294068, @mat wrote:
In D11865#294018, @tz wrote:

I think its ready for an exp-run. What do you think?

I do not think an exp-run would show anything. Unless it runs its bulk with -t, which would probably fail horribly, and early, not showing much.

Mh, yes, you are right.
So: do you approve it to be committed? ;)

I made a similar change to qa.sh and exp runs highlighted many issues.
I think an exp run is useful, but mat should know better.

Did you test it on at least 4 or 5 ruby ports?

FWIW this looks good to me. You still need ruby approval.

This revision is now accepted and ready to land.Jan 22 2018, 3:00 PM

Did you test it on at least 4 or 5 ruby ports?

I tested with www/redmine and www/gitlab. The second one is the most massive ruby port. I did not find any issues.

In D11865#294069, @tz wrote:

So: do you approve it to be committed? ;)

I did, on Friday.

I made a similar change to qa.sh and exp runs highlighted many issues.
I think an exp run is useful, but mat should know better.

Well, an exp-run will not run any QA checks, so this change, here, will be silently ignored.

This revision was automatically updated to reflect the committed changes.

Committed! Thanks to all of you for your help, time and feedback! :)