kubevirt / kubevirt.github.io

KubeVirt website repo, documentation at https://kubevirt.io/user-guide/
https://kubevirt.io
MIT License
30 stars 112 forks source link

Remove use of DEBUG env var from Makefile #864

Closed lyarwood closed 2 years ago

lyarwood commented 2 years ago

What this PR does / why we need it:

This conflicts with gems like html-proofer that use it to pull in dev dependencies [1][2].

[1] https://github.com/gjtorikian/html-proofer/commit/58c882ef1401aec2968c32efb85b299ae5d8b08e [2] https://github.com/gjtorikian/html-proofer/commit/2873f06e1ddd02b09274ad8825becc6540a688c1

Does this PR fix any issue? _(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged)_:

Fixes #862

Special notes for your reviewer:

cwilkers commented 2 years ago

Are you seeing a problem with DEBUG being set while building the container locally?

This doesn't affect the website CI build, which happens here: https://github.com/kubevirt/project-infra/blob/main/github/ci/prow-deploy/files/jobs/kubevirt/kubevirt.github.io/kubevirt-github-io-postsubmits.yaml#L41

lyarwood commented 2 years ago

Are you seeing a problem with DEBUG being set while building the container locally?

This doesn't affect the website CI build, which happens here: https://github.com/kubevirt/project-infra/blob/main/github/ci/prow-deploy/files/jobs/kubevirt/kubevirt.github.io/kubevirt-github-io-postsubmits.yaml#L41

Yes that's what I've used to reproduce this, our current Makefile sets DEBUG to @ that's enough to trigger html-proofer to pull in debug and awesome_print:

$ echo $DEBUG

$ bundle config set --local path 'vendor/bundle'
$ bundle install && make build
Fetching gem metadata from https://rubygems.org/..........
Resolving dependencies...
Using rake 13.0.6
Using asciidoctor 2.0.17
Using rainbow 3.1.1
Using yell 2.2.2
Using concurrent-ruby 1.1.10
Using rb-fsevent 0.11.1
Using http_parser.rb 0.8.0
Using zeitwerk 2.6.0
Using rouge 3.30.0
Using colorator 1.1.0
Using unicode-display_width 1.8.0
Using forwardable-extended 2.6.0
Using parallel 1.22.1
Using bundler 2.3.7
Using unicode_utils 1.4.0
Using ffi 1.15.5
Using i18n 1.12.0
Using terminal-table 2.0.0
Using pathutil 0.16.2
Using liquid 4.0.3
Using mercenary 0.4.0
Using ethon 0.15.0
Using sassc 2.4.0
Using rb-inotify 0.10.1
Using eventmachine 1.2.7
Using jekyll-paginate 1.1.0
Using em-websocket 0.5.3
Using nuggets 1.6.1
Using public_suffix 4.0.7
Using jekyll-tagging 1.1.0
Using addressable 2.8.0
Using premonition 2.0.1
Using jekyll-sass-converter 2.2.0
Using listen 3.7.1
Using rexml 3.2.5
Using racc 1.6.0
Using typhoeus 1.4.0
Using jekyll-watch 2.2.1
Using nokogiri 1.13.8 (x86_64-linux)
Using webrick 1.7.0
Using kramdown 2.4.0
Using safe_yaml 1.0.5
Fetching html-proofer 4.4.0
Using kramdown-parser-gfm 1.1.0
Using svg_optimizer 0.2.6
Using jekyll 4.2.2
Using jekyll-sitemap 1.4.0
Using jekyll-asciidoc 3.0.0
Using jekyll-diagrams 0.10.0
Using jekyll-feed 0.16.0
Using jekyll-inline-svg 1.1.4
Using jekyll-redirect-from 0.16.0
Installing html-proofer 4.4.0
Bundle complete! 14 Gemfile dependencies, 52 gems now installed.
Bundled gems are installed into `./vendor/bundle`

Makefile: Build jekyll site
scripts/update_changelog.sh
Already on 'main'
Your branch is up to date with 'origin/main'.
remote: Enumerating objects: 2, done.
remote: Counting objects: 100% (2/2), done.
remote: Compressing objects: 100% (2/2), done.
remote: Total 2 (delta 1), reused 1 (delta 0), pack-reused 0
Unpacking objects: 100% (2/2), 3.61 KiB | 1.80 MiB/s, done.
From https://github.com/kubevirt/kubevirt
 * [new tag]             v0.55.1      -> v0.55.1
 * [new tag]             v0.56.0-rc.1 -> v0.56.0-rc.1
Already up to date.
rake
rake aborted!
LoadError: cannot load such file -- debug
<internal:/usr/share/rubygems/rubygems/core_ext/kernel_require.rb>:85:in `require'
<internal:/usr/share/rubygems/rubygems/core_ext/kernel_require.rb>:85:in `require'
/home/lyarwood/.local/share/gem/ruby/gems/zeitwerk-2.6.0/lib/zeitwerk/kernel.rb:35:in `require'
/home/lyarwood/.local/share/gem/ruby/gems/html-proofer-4.4.0/lib/html_proofer.rb:19:in `<top (required)>'
/home/lyarwood/.local/share/gem/ruby/gems/html-proofer-4.4.0/lib/html-proofer.rb:3:in `require_relative'
/home/lyarwood/.local/share/gem/ruby/gems/html-proofer-4.4.0/lib/html-proofer.rb:3:in `<top (required)>'
<internal:/usr/share/rubygems/rubygems/core_ext/kernel_require.rb>:160:in `require'
<internal:/usr/share/rubygems/rubygems/core_ext/kernel_require.rb>:160:in `rescue in require'
<internal:/usr/share/rubygems/rubygems/core_ext/kernel_require.rb>:149:in `require'
/home/lyarwood/redhat/devel/src/k8s/kubevirt/kubevirt.github.io/Rakefile:3:in `block in <top (required)>'
/home/lyarwood/redhat/devel/src/k8s/kubevirt/kubevirt.github.io/Rakefile:2:in `<top (required)>'
/usr/share/gems/gems/rake-13.0.6/exe/rake:27:in `<top (required)>'

Caused by:
LoadError: cannot load such file -- html-proofer
<internal:/usr/share/rubygems/rubygems/core_ext/kernel_require.rb>:85:in `require'
<internal:/usr/share/rubygems/rubygems/core_ext/kernel_require.rb>:85:in `require'
/home/lyarwood/redhat/devel/src/k8s/kubevirt/kubevirt.github.io/Rakefile:3:in `block in <top (required)>'
/home/lyarwood/redhat/devel/src/k8s/kubevirt/kubevirt.github.io/Rakefile:2:in `<top (required)>'
/usr/share/gems/gems/rake-13.0.6/exe/rake:27:in `<top (required)>'
(See full trace by running task with --trace)
make: *** [Makefile:110: build] Error 1

$ git diff
diff --git a/Makefile b/Makefile
index f606f2c35..f368f3703 100644
--- a/Makefile
+++ b/Makefile
@@ -107,6 +107,7 @@ endif
 build: envvar
        @echo "${GREEN}Makefile: Build jekyll site${RESET}"
 #      which $(RUBY)
+       echo $(DEBUG)
        scripts/update_changelog.sh
        rake
        touch _site/.nojekyll
$ make build
[..]
Makefile: Build jekyll site
echo @
@
scripts/update_changelog.sh
Cloning into 'build/kubevirt'...
[..]

Obviously I think html-proofer is really at fault here but instead of waiting for a fix and release there we can just drop our use of the DEBUG env var for the time being.

cwilkers commented 2 years ago

I just updated the Dockerfile in project infra to use Fedora 35, and have done a bunch of testing with the kubevirt.github.io Makefile, everything should be working now -- despite the debug environment variable.

I think the problem with $DEBUG is valid though; if you would like to address that, I'd rather see the existing code changed to use an alternative name, for instance VERBOSE. You could then alter the logic such that:

ifdef DEBUG
 set VERBOSE := "@"
else
  set VERBOSE := ""
endif

...
rule:
    ${VERBOSE}commands
lyarwood commented 2 years ago

@cwilkers I see the build process is still broken for the site, I'm pretty sure this PR unblocks it FWIW.

cwilkers commented 2 years ago

We can try, but the issue was not in the same container are you're getting built in testing.

I have, however, done some work in the back end to swap to using that container for the site build AND updated the container itself to build against Fedora 36 instead of 34, which resolves lots of the dependency issues.

/lgtm /approve

kubevirt-bot commented 2 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cwilkers

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubevirt/kubevirt.github.io/blob/main/OWNERS)~~ [cwilkers] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
lyarwood commented 2 years ago

We can try, but the issue was not in the same container are you're getting built in testing.

I have, however, done some work in the back end to swap to using that container for the site build AND updated the container itself to build against Fedora 36 instead of 34, which resolves lots of the dependency issues.

/lgtm /approve

Cool either way the build is passing and the site has finally been updated, thanks!

cwilkers commented 2 years ago

I have to give you credit, the fix was all you (I have been working on that updated container, but just realized the code to use it has not been pushed yet.)

Thank you very much!