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 support for explicitly specifying some artifacts to add #41

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

doudou
Copy link

@doudou doudou commented Oct 31, 2019

The 'rice' gem has bunch of files in ruby/lib that is not part
of the gem proper - it is to be used by the libraries. It's
obviously a misuse of the Rubygems system, but a useful one
so far.

This adds the --artifact PATH argument that allows to:

  • add a file explicitly
  • add a complete directory tree

The 'rice' gem has bunch of files in ruby/lib that is not part
of the gem proper - it is to be used by the libraries. It's
obviously a misuse of the Rubygems system, but a useful one
so far.

This adds the --artifact PATH argument that allows to:
- add a file explicitly
- add a complete directory tree
@luislavena
Copy link
Owner

Hello @doudou, thank you for your PR and providing the background of this request.

I haven't looked into the code yet but planning this weekend. Will also try to correct the CI issues so is no longer showing incorrect errors.

Thanks for your patience.

Cheers.

@doudou
Copy link
Author

doudou commented Nov 6, 2019

Hi Luis.

Heads up that I'm already preparing a v2 version of this PR ... I never realized how powerful Dir.glob is, and am thinking of directly allowing passing globs instead of files.

@doudou
Copy link
Author

doudou commented Nov 7, 2019

Updated ...


(@options[:artifacts] || []).map do |include, glob|
resolved = Dir.glob("#{target_dir}/#{glob}")
if include
Copy link
Owner

Choose a reason for hiding this comment

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

Can I recommend any word other than include?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah. Missed that one. to_include ? to_be_included ? Or I could make it a mode variable that is either :include or :exclude. What do you think ?

@luislavena
Copy link
Owner

Hello @doudou, thanks for your patience.

Been looking at this change over the holidays and this might clash or could be refactor so --include-shared-dir can reuse this pattern globing option.

Will work on this over the holidays and submit an alternate version.

Cheers.

@luislavena
Copy link
Owner

Hello @doudou, thanks for your patience.

I was working on a different approach on this and wanted to check with you the exclude pattern functionality.

From this PR, you're removing files from artifacts, which is by default the platform specific files that were collected, but not necessarily impacting the files that are included in the gem package.

Should include and exclude patterns be applied to the final list of files and not just the collected artifacts? I'm seeing an scenario for this like the following:

$ gem compile mygem.gem --exclude 'ext' --exclude 'spec'

Let me know your thoughts.

Cheers.

@doudou
Copy link
Author

doudou commented May 21, 2020

Should include and exclude patterns be applied to the final list of files and not just the collected artifacts? I'm seeing an scenario for this like the following:

I don't see a problem with that.

@luislavena
Copy link
Owner

Excellent @doudou, wanted to confirm since your current PR applies this to collected artifacts and not all the files in the gemspec.

Will submit an alternative version in the upcoming days to address this.

Thank you for your patience and responses.

Cheers.

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