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

Add command-line option to force rebuilding specific packages #756

Merged
merged 2 commits into from
Apr 29, 2022

Conversation

TimoWilken
Copy link
Contributor

This is an alternative to adding force_rebuild: true to the alidist recipe, so that alidist doesn't have to be changed.

Example:

aliBuild build O2Physics --defaults o2 --force-rebuild O2 --force-rebuild O2Physics

(You can specify multiple packages to rebuild by giving the option multiple times. This makes it consistent with how --disable works.)

@adriansev: could you check whether this does what you need?

This is an alternative to adding `force_rebuild: true` to the alidist
recipe, so that alidist doesn't have to be changed.
@adriansev
Copy link

at least from description is what i need, but to actually install the alibuild in devel mode to check it out it will take some time ..
as a side comment, i'm surprised about the comment related to --disable i thought that it worked with --disable package1,package2,..

@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2022

Codecov Report

Merging #756 (d887edb) into master (35bf9aa) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head d887edb differs from pull request most recent head 2ebab24. Consider uploading reports for the commit 2ebab24 to get more accurate results

@@            Coverage Diff             @@
##           master     #756      +/-   ##
==========================================
+ Coverage   87.17%   87.18%   +0.01%     
==========================================
  Files          29       29              
  Lines        2970     2974       +4     
==========================================
+ Hits         2589     2593       +4     
  Misses        381      381              
Impacted Files Coverage Δ
alibuild_helpers/build.py 79.80% <ø> (ø)
tests/test_args.py 98.18% <ø> (ø)
tests/test_build.py 95.70% <ø> (ø)
alibuild_helpers/args.py 98.36% <100.00%> (+0.02%) ⬆️
alibuild_helpers/utilities.py 89.62% <100.00%> (+0.03%) ⬆️
alibuild_helpers/workarea.py 89.58% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@TimoWilken
Copy link
Contributor Author

TimoWilken commented Apr 19, 2022

at least from description is what i need, but to actually install the alibuild in devel mode to check it out it will take some time ..

OK, thanks @adriansev! Let me know when you've had time to try it.

as a side comment, i'm surprised about the comment related to --disable i thought that it worked with --disable package1,package2,..

You're right, that does work. I've made it consistent in the latest commit, so you can use --force-rebuild O2,O2Physics as well.

@adriansev
Copy link

so, i will just leave here as a log the problems encountered and at the end i will ping you...
so, installing with :
python3 -m pip install git+file:///home/physics-tools/alice/alibuild@test756
where test756 is the branch with the PR #756
gave me:

Building wheels for collected packages: alibuild
  Building wheel for alibuild (setup.py) ... done
  Created wheel for alibuild: filename=alibuild-LAST_TAG-py3-none-any.whl size=74958 sha256=a9f78247af9dc3c47718fae8b3e182e585ec4e0d49944c9733d8614d8f1ce9d1
  Stored in directory: /home/adrian/tmp/pip-ephem-wheel-cache-hdvovsx0/wheels/d9/cd/43/19c7e89174be81cc63b9f34d94338526fc22dcdf3a024cb13f
  WARNING: Built wheel for alibuild is invalid: Metadata 1.2 mandates PEP 440 version, but 'LAST-TAG' is not
Failed to build alibuild
Installing collected packages: alibuild
  Attempting uninstall: alibuild
    Found existing installation: alibuild 1.11.1
    Uninstalling alibuild-1.11.1:
      Successfully uninstalled alibuild-1.11.1
  Running setup.py install for alibuild ... done
  DEPRECATION: alibuild was installed using the legacy 'setup.py install' method, because a wheel could not be built for it. A possible replacement is to fix the wheel build issue reported above. Discussion can be found at https://github.com/pypa/pip/issues/8368

moreover, something is wrong with the shebang adaptation as i got this:

head  /home/adrian/.local/bin/aliBuild
#!python
from __future__ import print_function
import os
import sys

after modification by hand to a proper line (#!/usr/bin/env python3) the building started successfully

@adriansev
Copy link

@TimoWilken i can confirm that it works as advertised: after the initial build, another build command will tell me that Packages already built using this version but with force-rebuild it will actually rebuild the package. Thanks a lot!

@@ -89,14 +86,23 @@ def doParseArgs(star):
help=("Fetch updates to repositories in MIRRORDIR. Required but nonexistent "
"repositories are always cloned, even if this option is not given."))

build_parser.add_argument("--no-local", dest="noDevel", metavar="PKGLIST", default="", type=csv_list,
build_parser.add_argument("--no-local", dest="noDevel", metavar="PACKAGE", default=[], action="append",
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean it will not support comma separated lists anymore? I do not think this is acceptable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, 2ebab24 fixes this so that --no-devel and --force-rebuild work like --disable (in that --disable A,B and --disable A --disable B are equivalent).

@ktf ktf self-requested a review April 29, 2022 08:42
@@ -89,14 +86,23 @@ def doParseArgs(star):
help=("Fetch updates to repositories in MIRRORDIR. Required but nonexistent "
"repositories are always cloned, even if this option is not given."))

build_parser.add_argument("--no-local", dest="noDevel", metavar="PKGLIST", default="", type=csv_list,
build_parser.add_argument("--no-local", dest="noDevel", metavar="PACKAGE", default=[], action="append",
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean it will not support comma separated lists anymore? I do not think this is acceptable.

@ktf ktf merged commit fdf3f06 into alisw:master Apr 29, 2022
@TimoWilken TimoWilken deleted the force-rebuild-option branch April 29, 2022 09:11
@TimoWilken
Copy link
Contributor Author

I've released v1.11.3 with this change.

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.

None yet

4 participants