search

ManageIQ / polisher

Polisher is a Ruby module and set of utilities aimed to assisting the post-publishing packaging process for Ruby gems and applications.
MIT License
14 stars 14 forks source link

WIP: Support RubyGems without -doc subpackage #117

Closed strzibny closed 10 years ago

strzibny commented 10 years ago

Hi,

I continue on making auto-updates of specs possible. This PR deals with support for specs without -doc subpackage. It also fixes a few things on the way, namely:

This PR is not completely ready. I want to adjust (format) the test and test against the expected specs which should ensure that indexes are correct as well. Please review.

coveralls commented 10 years ago

Coverage Status

Coverage increased (+2.75%) when pulling a0bd3e6851c48c2ac299614f9abcc0e344cdd4d8 on strzibny:packages-without-doc-subpackage into 48990f5eac37d28c2dfb5da59297fae9181d4a05 on ManageIQ:master.

movitto commented 10 years ago

@strzibny Tried it out and looks good. Some comments:

  • rpmize: don't add %doc if it was added already (this was introduced by one of my commits)
  • delete the @update_gem mechanism as otherwise you cannot update to the specified .gem (it would always ask for upstream)

I'm a little unclear as to the actual problem. Polisher::RPM::Spec#@update_gem is used as a flag to indicated if polisher should retrieve the gem from rubygems or to use the local cached copy. If set to true (as occurs when the spec is updated) the version corresponding to that specified in the specfile will be retrieved. If false (which it is set to when the gem is retrieved), the local cached copy is used.

  • meta[:pkg_files] has to be set (maybe we can ensure it differently, ideas welcomed)

+1

  • include files in -doc subpackage only if it exists
  • doc_subpkg = @metadata[:contents].index RPM::Spec::SPEC_DOC_SUBPACKAGE_MATCHER
  • package = doc_subpkg ? 'doc' : pkg

Not a fan of running this search at this point (potentially every iteration of the files loop). At the very least we should move this variable assignment to the top of the #update_files_from method. Ideally we can store the subpackages defined in the spec metadata when parsing it, and then provide a simple #has_subpkg?(name) helper method

  • correctly count indexes if -doc subpackage is missing and/or requires are missing (for fedora 21 and higher we auto-generate the requires and provides)

Ah good catch.

  • add specs for updating spec files

Works for me. Note some of the style notes can be ignored (void operator and such), hopefully can simplify how we do the style / clean some of it up going forward (see #118)

Overall looks great, thanks for the changes.

strzibny commented 10 years ago

Thanks for the comments.

I'm a little unclear as to the actual problem. Polisher::RPM::Spec#@update_gem is used as a flag to indicated if polisher should retrieve the gem from rubygems or to use the local cached copy. If set to true (as occurs when the spec is updated) the version corresponding to that specified in the specfile will be retrieved. If false (which it is set to when the gem is retrieved), the local cached copy is used.

The thing is that the invalidation happens in #update_metadata_from which happens way after #update_deps_from (which calls #upstream_gem) so it confused me. And if we move the invalidation up to the #update_to, we can't set the updated gem. So yes, I misunderstood it a bit the first time. My goal is to be able to set the .gem file locally, so in the specs I could write:

rpmspec = File.read(File.expand_path(File.join(File.dirname(__FILE__), '../data/rubygem-activesupport.spec')))
rpmspec = Polisher::RPM::Spec.parse rpmspec
# set the corresponding gem so the gem won't be downloaded
rpmspec.gem = @original_source
rpmspec.update_to(@source)

This can be done easily by having attr_accessor for the gem:

module Polisher
  module RPM
    module HasGem
      # .gem file associated with the RPM specfile
      attr_accessor :gem

      # Return gem corresponding to spec name/version
      def upstream_gem
        @gem, @update_gem = nil, false if @update_gem
        @gem ||= Polisher::Gem.from_rubygems gem_name, version
      end

And it actually works this way so I could keep the @gem, @update_gem = nil, false if @update_gem because the invalidation happens later in #update_metadata_from. Is this the way it works?

But then, why not to rather override the @gem with the new sources in #update_to? Would we need the invalidation then? So after the update is over we can ask rpmspec.gem or rpmspec.upstream_gem again and get the new source without any download.

Not a fan of running this search at this point (potentially every iteration of the files loop)

Oh yes, let's change it.

movitto commented 10 years ago

The thing is that the invalidation happens in #update_metadata_from which happens way after

update_deps_from (which calls #upstream_gem) so it confused me.

@strzibny memory is a bit hazy but believe we want to have the 'update_deps_from' method rely on the original gem version there, since the calls it dispatches to are 'excludes_dep?' and 'excludes_dev_dep?' (which in return dispatch to 'upstream_gem'). Would be best to analyze the current gem there, to determine which requires & build have been excluded (so as to omit those gem deps from the updated spec as well)

My goal is to be able to set the .gem file locally, so in the specs I could write:

You could stub out the 'upstream_gem' method via rspec and return whichever value you want (eg a custom Gem instance or whatever) but I'd be fine w/ adding an accessor if it'd help in this use case.

coveralls commented 10 years ago

Coverage Status

Coverage increased (+2.77%) when pulling 6f13c4ae17bec0ec501e2dac8e80027ae694f51e on strzibny:packages-without-doc-subpackage into f19f9776f3d01e66802ae01c860635fa07fc706c on ManageIQ:master.

coveralls commented 10 years ago

Coverage Status

Coverage increased (+2.77%) when pulling 28c8687457a95226ee9bd36931464b9cf983c2db on strzibny:packages-without-doc-subpackage into f19f9776f3d01e66802ae01c860635fa07fc706c on ManageIQ:master.

coveralls commented 10 years ago

Coverage Status

Coverage increased (+2.77%) when pulling a1023dd3b7b7f9d26c5832935b1a4c14f648811e on strzibny:packages-without-doc-subpackage into f19f9776f3d01e66802ae01c860635fa07fc706c on ManageIQ:master.

coveralls commented 10 years ago

Coverage Status

Coverage increased (+2.77%) when pulling 95a732bc48cdac4a5d7600650cdad060af8e6c1d on strzibny:packages-without-doc-subpackage into f19f9776f3d01e66802ae01c860635fa07fc706c on ManageIQ:master.

strzibny commented 10 years ago

You could stub out the 'upstream_gem' method via rspec and return whichever value you want (eg a custom Gem instance or whatever) but I'd be fine w/ adding an accessor if it'd help in this use case.

I know, but this can be helpful in scripts (thinking offline as well), so I kept the accessor.

I also moved the logic of has_doc_subpkg? to appropriate place and formatted the specs.

miq-bot commented 10 years ago

Checked commit https://github.com/strzibny/polisher/commit/95a732bc48cdac4a5d7600650cdad060af8e6c1d with rubocop 0.21.0 6 files checked, 20 offenses detected

lib/polisher/rpm/spec.rb

spec/rpm/updates_spec_spec.rb

movitto commented 10 years ago

@strzibny srry for belated response. Looks good to me, merging.