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

Makefile: various improvements #495

Open
wants to merge 5 commits into
base: gh-pages
Choose a base branch
from

Conversation

maxim-belkin
Copy link
Contributor

@maxim-belkin maxim-belkin commented Aug 17, 2020

  1. Use proper bundle config set --local path ... syntax instead of (incorrect) bundle config --local set path ... (implemented in Makefile: fix 'bundle config' command flags #559).
  2. Decide whether to use bundle or jekyll based on the presence of Gemfile or Gemfile.lock
  3. Use $(SHELL) to call bin/knit_lessons.sh: this lets us avoid relying on /usr/bin/env finding bash. (implemented in Makefile: use SHELL to call bin/knit_lesson.sh #562)
  4. New bundle target for building .vendor/bundle directory with required gems:
    a. This is an alias for .vendor/bundle target which depends on Gemfile and Gemfile.lock
    b. It installs/updates bundled gems only if necessary
  5. Empty Gemfile target for lessons that don't use it.
    a. Gemfile.lock target that creates it based on Gemfile
  6. clean target: remove .vendor and .bundle directories and Gemfile.lock file. (implemented in Makefile: better cleanup #560)

@maxim-belkin maxim-belkin marked this pull request as draft August 18, 2020 15:14
@maxim-belkin maxim-belkin marked this pull request as ready for review August 18, 2020 18:28
@maxim-belkin maxim-belkin force-pushed the knit-shell branch 2 times, most recently from e3032cc to 153729d Compare August 21, 2020 18:16
@maxim-belkin maxim-belkin changed the title Makefile: inprovements for bundle Makefile: improvements for bundle Aug 22, 2020
@maxim-belkin
Copy link
Contributor Author

@fmichonneau, I rebased this on top of the latest gh-pages. Changes here deal with Bundler and Gemfile(s). This PR also fixes an incorrect syntax for bundler config command (which should be bundle config set --local rather than bundle config --local set).

@fmichonneau
Copy link
Contributor

Thanks for this Maxim!
Overall, it looks good.
After talking with the @carpentries/core-team-curriculum, one thing that came up is that we do want everyone to use bundler to ensure that we use the same dependencies as GitHub pages, so when the Gemfile is missing or bundler is not available, the build should fail and indicate that these components are missing.
I think we also want to run bundle update regularly (every time?) to ensure that dependencies get updated when the github-pages gem gets updated.

@maxim-belkin
Copy link
Contributor Author

we do want everyone to use bundler to ensure that we use the same dependencies as GitHub pages, so when the Gemfile is missing or bundler is not available, the build should fail and indicate that these components are missing.

Sounds reasonable. I've updated the PR accordingly.

I think we also want to run bundle update regularly (every time?) to ensure that dependencies get updated when the github-pages gem gets updated.

In the light that we want to be able to run make serve and make site offline (#451 (comment)) and that bundle update requires an internet connection, I split bundle target into two:

  1. bundle target that installs all the necessary gems into .vendor/bundle directory, and
  2. update-bundle target that updates gems in .vendor/bundle directory.

make site uses gems currently installed in .vendor/bundle (if it exists) or installs them there. This way, one can run make bundle while connected to the internet, then use make site offline. Then, whenever they're online and want to update the gems, they can call make update-bundle. Does that seem reasonable? Please let me know if you think that make site has to call bundle update regardless of the internet connection -- I'll update the PR.

@maxim-belkin maxim-belkin force-pushed the knit-shell branch 2 times, most recently from 6a81fa5 to 3f73ef2 Compare August 26, 2020 21:43
@maxim-belkin
Copy link
Contributor Author

@fmichonneau, do you have any concerns or questions about this PR?

@maxim-belkin
Copy link
Contributor Author

maxim-belkin commented Oct 21, 2020

@fmichonneau, ping. If desired, I can factor out the fix for the bundle config command into a separate PR -- just let me know.

@fmichonneau
Copy link
Contributor

Thanks for the ping @maxim-belkin, this had fell off my radar.
I appreciate the flexibility that separating the update-bundle and site targets provide but I worry that people will not forget to run update-bundle regularly. Could we keep make site to run bundle update but maybe create a new target make site-offline that would not (and say that an internet connection is needed if the bundle doesn't exist)?

@maxim-belkin
Copy link
Contributor Author

I appreciate the flexibility that separating the update-bundle and site targets provide but I worry that people will not forget to run update-bundle regularly.

update-bundle target (which runs bundle update) updates the Gemfile.lock file when GitHub updates the github-pages gem. The release cycle of this gem is irregular and isn't frequent (see https://github.com/github/pages-gem/releases), so I wouldn't worry about not running bundle update every time we build a website locally. But even when the gem is updated -- I don't expect our lessons to depend on the bleeding-edge features/gems.

Could we keep make site to run bundle update but maybe create a new target make site-offline that would not (and say that an internet connection is needed if the bundle doesn't exist)?

We can:

  1. create another target that would do that
  2. use some tricks to run bundle update monthly/weekly/daily
  3. leave this PR as is

Personally, I think make site should be as "offline" as possible because it's used to build a website and not be 100% in sync with the github-pages gem. But if you think we need to run bundle update with every make site -- I'd be happy to do that.

@maxim-belkin
Copy link
Contributor Author

Also, The Carpentries could remind maintainers to update the bundles (by running make update-bundle or bundle update) in monthly emails when the github-pages gem is updated.

@maxim-belkin
Copy link
Contributor Author

@fmichonneau, I added site-offline target and made site target always update the bundle.

@maxim-belkin
Copy link
Contributor Author

Failure (R Intro Geospatial (macOS), link) seems to be unrelated to the change.

@maxim-belkin
Copy link
Contributor Author

I'm going to move the fix for thebundle config command into a separate PR as it seems that the review of this PR is taking a bit longer than usual.

@maxim-belkin maxim-belkin force-pushed the knit-shell branch 7 times, most recently from 20f284a to 300b50a Compare March 24, 2021 15:34
1. Internal 'bundler' target to check that Bundler is installed

2. 'bundle' target for building required gems into the `.vendor/bundle` directory:
    a. This is an alias for `.vendor/bundle` target which depends on Gemfile and Gemfile.lock
    b. It installs/updates bundled gems only if necessary

3. Gemfile target to check that Gemfile is present
   Gemfile.lock target
@maxim-belkin
Copy link
Contributor Author

@fmichonneau @zkamvar, I've implemented several improvements to this PR and I think it's ready for your reviews.
Below I list the most noticeable changes:

  1. Added $(info ...) to all targets to make it clear which one is being executed:

    Example: make site
    $ make site
    ==> bundler
    ==> Gemfile.lock [Gemfile]
    Fetching gem metadata from https://rubygems.org/...........
    Fetching gem metadata from https://rubygems.org/.
    Resolving dependencies....
    Writing lockfile to /Users/mbelkin/git/swcarpentry/python-novice-inflammation/Gemfile.lock
    ==> .bundle/config []
    ==> update-bundle [Gemfile.lock .bundle/config]
    ==> lesson-md []
    ==> site-offline [lesson-md index.md bundler .vendor/bundle]
    Configuration file: /Users/mbelkin/git/swcarpentry/python-novice-inflammation/_config.yml
                Source: /Users/mbelkin/git/swcarpentry/python-novice-inflammation
           Destination: /Users/mbelkin/git/swcarpentry/python-novice-inflammation/_site
     Incremental build: disabled. Enable with --incremental
          Generating... 
                        done in 3.291 seconds.
     Auto-regeneration: disabled. Use --watch to enable.
    ==> site [update-bundle site-offline]
    
    Example: make site-offline
    $ make site-offline
    ==> lesson-md []
    ==> bundler
    ==> site-offline [lesson-md index.md bundler .vendor/bundle]
    Configuration file: /Users/mbelkin/git/swcarpentry/python-novice-inflammation/_config.yml
                Source: /Users/mbelkin/git/swcarpentry/python-novice-inflammation
           Destination: /Users/mbelkin/git/swcarpentry/python-novice-inflammation/_site
     Incremental build: disabled. Enable with --incremental
          Generating... 
                        done in 3.335 seconds.
     Auto-regeneration: disabled. Use --watch to enable.
    
  1. Added site-offline and serve-offline targets. These targets do not update bundled Ruby gems before building/serving a website.

  2. clean target now deletes Gemfile.lock only if Git does not track it.

  3. New targets related to bundler and gems:

    1. bundler: check that Bundler is installed
    2. .bundle/config: configures Bundler (bundle config set --local path .vendor/bundle)
    3. install-bundle: install Ruby gems specified in Gemfile/Gemfile.lock
    4. .vendor/bundle: same as install-bundle (but not a PHONY target)
    5. update-bundle: update Ruby gems specified in Gemfile/Gemfile.lock. Used by site and serve targets
    6. Gemfile.lock: create Gemfile.lock based on Gemfile
    7. Added index.html target (necessary for workshop-check) and combined it with index.md and Gemfile targets: each of these targets does one thing -- checks that corresponding file exists, so it makes sense to not duplicate code.
    8. Some targets use bundler as an order-only prerequisite (| bundler). This is necessary because bundler is a PHONY target and when it's used as a normal prerequisite it causes the target to be regenerated even though there were no changes.

And it looks like these changes passed all the tests while I was writing all of the above, so this is promising.

@maxim-belkin maxim-belkin changed the title Makefile: improvements for bundle Makefile: various improvements Jun 29, 2021
Technically, prerequisites can be executed in arbitrary order, so
we can't rely on 'update-bundle' prerequisite being executed before
'serve-offline' or 'site-offline' prerequisites. Therefore, we have to
decouple these targets.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants