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

Improvements #60

Merged
merged 10 commits into from
Mar 25, 2021
Merged

Improvements #60

merged 10 commits into from
Mar 25, 2021

Conversation

lslezak
Copy link
Member

@lslezak lslezak commented Mar 24, 2021

Here are some small improvements for the rake tasks:

Added osc:config task

The packaging configuration might be complex and it might be quite difficult to find where the package will be submitted with the osc:sr task. This task prints the summary of the packaging configuration:

# rake osc:config
Package directory: package
OBS instance:      https://api.opensuse.org/
OBS project:       devel:languages:ruby:extensions
OBS package name:  rubygem-packaging_rake_tasks
OBS build target:  openSUSE_Tumbleweed
OBS submit target: openSUSE:Factory

It also honors the YAST_SUBMIT variable in the YaST packages:

# YAST_SUBMIT=sle15sp1 rake osc:config
Package directory: package
OBS instance:      https://api.suse.de/
OBS project:       Devel:YaST:SLE-15-SP1
OBS package name:  yast2
OBS build target:  SUSE_SLE-15-SP1_Update
OBS submit target: SUSE:SLE-15-SP1:Update

Git Commit Check

The osc:build or osc:sr tasks check whether all changes have been committed to Git to avoid missing files and changes. That's good. But if you are testing a fix it is quite annoying that you have to commit every single change before running a testing build.

Also I'd like to use it in the rake task for running the GitHub Actions locally, forcing developers to commit every change before running the tests would be really annoying...

Usage:

rake osc:build CHECK_MODIFIED=0

Note: We need to keep the check for new files because rake package archives only the files known to Git, so the package build would miss them.

Other Changes

  • Use syntax highlighting in the README.md file
  • The rake install now uses sudo to make package building and installing easier
  • Fixed several spelling issues

README.md Outdated
@@ -78,9 +82,20 @@ basesystem for more efficient caching.
An optional argument is passed as options to `osc` and can be used to prefer
local packages: `rake "osc:build[-p /var/tmp/YaST:Head/openSUSE_Factory]"`

This task checks whether all changes are committed to Git (using the
[check:committed](#checkcommitted) task). If you want to build the package
with your local changes without committing to Git set `COMMIT_CHECK` to `0`:
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure how it will work with removed/added files.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point... 😟

Copy link
Member

Choose a reason for hiding this comment

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

So please document it in README as caveeat as it can surprise users. Otherwise change looks good.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

Rakefile Outdated
@@ -69,7 +69,7 @@ end

desc 'Install packaging_rake_tasks gem package'
task :install => :tarball do
sh 'gem install package/packaging_rake_tasks*.gem'
sh 'sudo gem install --local package/packaging_rake_tasks*.gem'
Copy link
Member

Choose a reason for hiding this comment

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

to be honest I do not like much this change as forces sudo usage and on some machines sudo is not even available. for make install you also need to use sudo make install

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a difference to make, first you call make then sudo make install so only the install part runs with the root permissions.

If you run sudo rake install here it will also create the *.gem file as root. That's bad, I'd like to run only the really necessary steps as root, if there is a bug in creating the gem file then you could break your system easily.

Moreover if you want to delete the gem you also need sudo:

# ls -l package/*.gem
-rw-r--r-- 1 root root 23552 Mar 24 17:52 package/packaging_rake_tasks-1.5.0.gem

I'dd add root and sudo detection....

README.md Outdated
It doesn't check if changes
are sent to a remote git repository. Its main intention is to ensure that all
changes are tracked before making a package.

Note: The check is skipped when environment variable `COMMIT_CHECK` is set to `0`.
Copy link
Member

Choose a reason for hiding this comment

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

What about naming the option after the task, CHECK_COMMITTED ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It turned out that we need to keep the check for new files because rake package archives only the files known to Git, so the package build would miss them.

So we can only skip the check for modified files. But that would be still sufficient in many situations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's use CHECK_MODIFIED instead...

@@ -69,7 +69,8 @@ end

desc 'Install packaging_rake_tasks gem package'
task :install => :tarball do
sh 'sudo gem install --local package/packaging_rake_tasks*.gem'
sudo = (!Process.euid.zero? && File.exists?("/usr/bin/sudo")) ? "/usr/bin/sudo" : ""
sh "#{sudo} gem install --local package/packaging_rake_tasks*.gem"
Copy link
Member

Choose a reason for hiding this comment

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

another option is to use su -c in case sudo is not available

Copy link
Member Author

Choose a reason for hiding this comment

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

This rake install task is called only by developers when testing the gem locally, I'd keep it simple...

README.md Outdated Show resolved Hide resolved
Copy link
Member

@mvidner mvidner left a comment

Choose a reason for hiding this comment

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

Just one place in the docs has escaped the renaming, otherwise LGTM
Thanks!

lslezak and others added 2 commits March 25, 2021 07:53
Co-authored-by: Martin Vidner <mvidner@suse.cz>
@lslezak lslezak merged commit 5dbca7b into master Mar 25, 2021
@lslezak lslezak deleted the improvements branch March 25, 2021 07:52
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

3 participants