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

Provide a way to ignore files in a hex package upload #506

Closed
dmlittle opened this issue Feb 15, 2018 · 16 comments
Closed

Provide a way to ignore files in a hex package upload #506

dmlittle opened this issue Feb 15, 2018 · 16 comments

Comments

@dmlittle
Copy link
Contributor

There currently is no solution to exclude files that should be uploaded into the package manager when releasing a version of a package. The default values will pick up files that might be created by the OS or editor while working on the package that is being released.

For example, in macOS the Finder will create a .DS_Store file in each directory that has been viewed through it. If I ever opened any of the directories within the lib/ with Finder, the default files will pick up the .DS_Store files that were created (for example a lib/.DS_Store).

Below is an example of the files being included when publishing a recent package. You can notice two .DS_Store files that ideally shouldn't be uploaded into Hex.

$ mix hex.publish package
Building lob_elixir 0.2.0
  Dependencies:
    poison ~> 3.1 (app: poison)
    httpoison ~> 0.13 (app: httpoison)
  App: lob_elixir
  Name: lob_elixir
  Files:
    lib
    lib/.DS_Store
    lib/lob
    lib/lob/.DS_Store
    lib/lob/address.ex
    lib/lob/area.ex
    lib/lob/bank_account.ex
    lib/lob/check.ex
    lib/lob/client.ex
    lib/lob/intl_verification.ex
    lib/lob/letter.ex
    lib/lob/postcard.ex
    lib/lob/resource_base.ex
    lib/lob/route.ex
    lib/lob/us_verification.ex
    lib/lob/us_zip_lookup.ex
    lib/lob/util.ex
    mix.exs
    README.md
    CHANGELOG.md
  Description: Lob Elixir Library.
  Version: 0.2.0
  Build tools: mix
  Licenses: MIT
  Maintainers: Lob
  Links:
    GitHub: https://github.com/lob/lob-elixir
  Elixir: ~> 1.4
Before publishing, please read the Code of Conduct: https://hex.pm/policies/codeofconduct

Publishing package to public repository hexpm.
Proceed? [Yn] n

While it is true that these files being uploaded into Hex, they also serve absolutely no purpose. It could also be argued that some might consider it unprofessional to have files not really associated with the package be uploaded as part of the package. It is also true that a developer could be careful and make sure they don't upload files they don't intend to upload but I believe being able to configure these files knowing that they will exists is also a good experience.

I know that the .DS_Store example is very specific, but I'm sure there are other files that it makes sense to potentially exclude.

Other package manager solutions

  • npm uses a combination of two files, a local .gitignore and a .npmignore file, to exclude files to be uploaded
  • pip uses commands in the MANIFEST.in file to exclude files to uploaded
  • rebar3 has an option exclude_files to list file to be excluded from the upload

Proposition

Add an exclude_files option to the the package part of the mix.exs. Given that there already exists a files option, it makes sense to have both the included and excluded files live in one place.

@supersimple
Copy link
Member

One other idea would be to add a config option (in the hex.config) file. That would then make a global hex rule for any packages that user creates.
The config is also very out-of-the-way, so most users will not know about it and it wont be an obstacle.
mix hex.config ignore-files [".DS_Store", ".tool-versions", ...]

@ericmj
Copy link
Member

ericmj commented Feb 15, 2018

I think an exclude_files in the project config and/or in the global hex.config would be nice.

@wojtekmach
Copy link
Member

wojtekmach commented Feb 15, 2018

The problem with exclude_files is that it's a blacklist and usually we want to use whitelists.

I mentioned this in passing in other tickets so let me repeat here. I'd like to discuss solving the problem of unneeded files with a whitelist approach using either (or some combination of):

  1. changing default glob pattern from ["lib/*", ...] to be stricter: ["lib/*.ex", ...].

    the obvious caveat is it could still catch unwanted files, like tmp: ~foo.ex etc.

  2. using git ls-files to grab the list of files and put it into :files configuration on compilation time. see e.g.: https://github.com/ruby/rake/blob/v12.3.0/rake.gemspec#L27. This is something users can do themselves with no changes to Hex.

  3. have Manifest.txt file e.g.: https://github.com/seattlerb/minitest/blob/master/Manifest.txt. The idea is we'd only publish files that are in the manifest which is tracked in e.g. git. Thus, by design we'd never publish unwanted files because they'd need to be included into manifest in a separate step first. I haven't look closely how they handle it (I presume when project is being built it automatically adds lib/*.rb files to the manifest because adding them manually would be cumbersome). This option doesn't seem to be particularly popular but I'd like to explore it too.

thoughts?

@ericmj
Copy link
Member

ericmj commented Feb 15, 2018

exclude_files would complement include_files, so that we first add all include files and then remove excluded files.

git ls-files would unfortunately include unnecessary files.

The manifest seems like too much work and I don't see how we can automate it. Too many mistakes
will happen if the automate functionality is not perfect. I would rather have .DS_Store in all packages than 1% of packages being broken when published because they forget to manually add a file to the manifest.

@wojtekmach
Copy link
Member

wojtekmach commented Feb 15, 2018

git ls-files would unfortunately include unnecessary files.

we could do git ls-files lib/* etc, which should only include what we want (and if not, we put it into .gitignore). However, if say we keep src/*.xrl in git, gitignore src/*.erl and actually want not to ship *.xrl but do ship *.erl - it might be tricky.

here's one idea how manifest could work:

instead of files: ["lib/", "priv/"] the user specifies: manifest: ["lib/*.ex", "priv/assets/*.js"]. there could be a manifest compiler (which could be just compiler.manifest alias) that just writes files to manifest.txt. This way, when we commit we'd see a diff on manifest.txt anytime we've added or removed files that were caught by the glob. Which could get annoying but a) depends per project, but as library matures we seldom add new files b) it would definitely have prevented shipping bad ex_doc release because we'd see see in manifest diff that we no longer ship assets.

Ok, let's go with exclude_files and I can explore manifest as a standalone library (which it should be in the first place). sorry for noise!

(edit: updated paragraph about manifest being annoying)

@josevalim
Copy link
Member

But what will be exclude_files? Is it a regex? a pattern? Is it configured globally?

@wojtekmach
Copy link
Member

if anyone is interested, I've put together manifest package that handles manifest.txt 😂

@ericmj
Copy link
Member

ericmj commented Feb 16, 2018

But what will be exclude_files? Is it a regex? a pattern? Is it configured globally?

It should probably behave the same as files does currently. Since this is a list of strings we can't support it in hex.config currently, so I would suggest only adding it to the package config.

@josevalim
Copy link
Member

@ericmj then how would you ignore every .DS_STORE? Would you have to list all of them? Would you make it a glob?

@ericmj
Copy link
Member

ericmj commented Feb 16, 2018

It would be a glob as: **/.DS_Store.

@josevalim
Copy link
Member

@ericmj i wonder if exclude_patterns with regexes would be better.

@ericmj
Copy link
Member

ericmj commented Feb 16, 2018

Let's go with exclude_pattern as @josevalim suggested initially and let's only do it in the package config. We can extend on this later.

@josevalim
Copy link
Member

exclude_patterns, plural, if it is a list. :)

@nox
Copy link

nox commented Feb 16, 2018

@dmlittle
Copy link
Contributor Author

@ericmj I can work on implementing this feature. I'll have a PR later this week adding the exclude_patterns functionality.

@dmlittle
Copy link
Contributor Author

dmlittle commented Mar 5, 2018

Closing this issue as a PR adding this functionality has been merged! 🙌

@dmlittle dmlittle closed this as completed Mar 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants