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

General improvements #89

Closed
wants to merge 4 commits into from
Closed

Conversation

manuelnaranjo
Copy link
Collaborator

This PR bundles a bunch of non related changes I've been working on while getting ready for bzlmod support with a lock file.

It includes:

  • allow to configure the log level (logrus) through an environmental variable, so we don't need to rebuild the binary each time.
  • allow to configure the rpm database cache path, this cache by default is stored relatively to the path where the binary gets called, but this means it can't be shared across multiple workspaces using bazeldnf and the same repos, and also that when multiple OSes are being used conflicts can arise (for instance when mixing centos7 and centos stream 9 while doing a migration). By making the path configurable and relative to XDG paths we can get some isolation. This cache is not used by bazel operations, but by bazeldnf binary when resolving dependency trees.
  • make sure noarch packages are passed to the resolver, it's fine for an rpm for a given architecture to depend on a noarch package, this is a standard practice when large data files are required, we need to make sure the reducer doesn't ignore those packages and pass them into the SAT solver.

Now the logrus level can be changed through the  environment variable
LOG_LEVEL
Now the cache dir is global and can be configured properly, by default
it goes into XDG_CACHE_HOME/bazeldnf, this is useful when dealing with
multiple bazel repositories using bazeldnf.
Now the cache will expand paths to honor ~
When getting an arch make sure to expand to noarch as well, because some
RPMs may have dependencies that have no architecture, this seems to be
how RPM work
@manuelnaranjo manuelnaranjo self-assigned this Jan 15, 2025
@manuelnaranjo manuelnaranjo added the bzlmod bzlmod support related tasks label Jan 15, 2025
cacheDir = append(cacheDir, cacheHelperValues.cacheDir);
} else if (len(cacheDir) > 1) {
func expand(path string) (string, error) {
if len(path) == 0 || path[0] != '~' {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe the recommended approach here is to:
(1) replace ~ with $HOME
(2) os.Expand(path)

This will cover more than just tilde expansion because it'll substitute all of the local environment variables.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea, I'll do that

Copy link
Collaborator

@kellyma2 kellyma2 left a comment

Choose a reason for hiding this comment

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

Can we peel this last commit off for now? I realize I've been slow, but I'm going to merge my refactoring PR (assuming it's good to go from your end) and push a PR for tests around this stuff and I'd like to not break that.

@manuelnaranjo
Copy link
Collaborator Author

Can we peel this last commit off for now? I realize I've been slow, but I'm going to merge my refactoring PR (assuming it's good to go from your end) and push a PR for tests around this stuff and I'd like to not break that.

Sure, I was going to wait for you to merge first anyway

@manuelnaranjo
Copy link
Collaborator Author

I'll close this PR and move it to my company fork by now, as I can't push force into the branch and fix the commits

This was referenced Jan 17, 2025
@manuelnaranjo manuelnaranjo deleted the mnaranjo/prepare-for-lock-file branch January 19, 2025 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bzlmod bzlmod support related tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants