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

sqitch 1.3.1 (new formula) #129128

Closed
wants to merge 1 commit into from
Closed

sqitch 1.3.1 (new formula) #129128

wants to merge 1 commit into from

Conversation

theory
Copy link
Contributor

@theory theory commented Apr 23, 2023

Based on the sqitchers/sqitch tap, it supports PostgreSQL, MySQL, and SQLite out of the box, and Snowflake, Exasol, and Vertica with the installation of additional client libraries. Snowflake support could use the snowflake-snowsql formula, but it's Cask only. The caveat output points to a URL that describes how to install these additional dependencies, which will be updated appropriately by sqitchers/sqitch.org#13.

The install method installs all Perl modules in the Cellar directory, which is useful for keeping everything together and avoiding conflicts with other Perl applications, but means that they must all be re-installed when upgrading to a new version.

Not supported: Oracle and Firebase, since there are no formulas for Instant Client or the Firebase server (or clients). The URL emitted by the Caveat points users who need to manage Oracle or Firebase databases to the Sqitch tap.

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

@github-actions github-actions bot added automerge-skip `brew pr-automerge` will skip this pull request new formula PR adds a new formula to Homebrew/homebrew-core perl Perl use is a significant feature of the PR or issue labels Apr 23, 2023
@theory
Copy link
Contributor Author

theory commented Apr 23, 2023

 ==> An exception occurred within a child process:
    Errno::EACCES: Permission denied @ rb_sysopen - /opt/homebrew/etc/sqitch/templates/deploy/exasol.tmpl

Hrm. Sqitch installs templates in the etc directory. Maybe it needs to keep them in its own cellar directory? Not sure why it's only an issue on macOS 11 ARM64 tho. Suggestions?

@theory
Copy link
Contributor Author

theory commented Apr 23, 2023

It builds with

perl Build.PL --install_base /opt/homebrew/Cellar/sqitch/1.3.1 --etcdir /opt/homebrew/etc/sqitch --verbose

Is the installer not allowed to write files to the path specified by the etc variable? These are editable by the user and a reinstall should not replace them; putting them in the cellar would break that.

@ZhongRuoyu ZhongRuoyu changed the title Add formula for Sqitch sqitch 1.3.1 (new formula) Apr 23, 2023
depends_on "libpq"
depends_on "mysql-client"
depends_on "perl"
uses_from_macos "sqlite"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this from depends_on at the recommendation of brew audit --new, but as there is an sqlite formula that will install a newer version than on the OS, ISM it might be better kept depends_on.

Based on the sqitchers/sqitch tap, it supports PostgreSQL, MySQL, and
SQLite out of the box, and Snowflake, Exasol, and Vertica with ODBC via
the installation of additional client libraries. Snowflake support could
use the `snowflake-snowsql` formula, but it's Cask only. The caveat
output points to a URL that describes how to install these additional
dependencies, which will be updated appropriately by
sqitchers/sqitch.org#13.

The install method installs all Perl modules in the Cellar directory,
which is useful for keeping everything together and avoiding conflicts
with other Perl applications, but means that they must all be
re-installed when upgrading to a new version.

Not supported: Oracle and Firebase, since there are no formulas for
Instant Client or the Firebase server (or clients). The URL emitted by
the Caveat points users who need to manage Oracle or Firebase databases
to the Sqitch tap.
Formula/sqitch.rb Show resolved Hide resolved
Comment on lines +22 to +25
# Download Module::Build and Menlo::CLI::Compat.
cpm_args = %w[install --local-lib-contained instutil --no-test]
cpm_args.push("--verbose") if verbose?
system "cpm", *cpm_args, "Menlo::CLI::Compat", "Module::Build"
Copy link
Member

Choose a reason for hiding this comment

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

These should be resources instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean like this? Woof, that would be a lot.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, otherwise it could be installing anything and we want versioned dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I'm working on something to generate it from that cpan file. I presume I can put additional dependencies required to build --HEAD inside a if build.head? block and they'll only be downloaded for head builds, yes?

Comment on lines +70 to +77
def caveats
<<~EOS
This Sqitch install includes clients to manage Postgres, MySQL, and SQLite.
See https://sqitch.org/download/macos/ to install the client libraries
required to manage Snowflake, Exasol, and Vertica. Use the sqitchers/sqitch
tap to manage Oracle or Firebird.
EOS
end
Copy link
Member

Choose a reason for hiding this comment

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

We don't want external references here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hrm. Should I instead throw in a file and tell the user to read it for more details?

Copy link
Member

Choose a reason for hiding this comment

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

A lot of users skip the caveats, might be easier to just put something to this extend in your documentation.

end

test do
system bin/"sqitch", "--version"
Copy link
Member

Choose a reason for hiding this comment

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

We need a test that exercises the some of the functionality of the app. Version checks or usage checks (foo --version or foo --help) are not sufficient, as explained in the formula cookbook.

In most cases, a good test would involve running a simple test case: run #{bin}/foo input.txt.

  • Then you can check that the output is as expected (with assert_equal or assert_match on the output of shell_output)
  • You can also check that an output file was created, if that is expected: assert_predicate testpath/"output.txt", :exist?

Some advice for specific cases:

  • If the formula is a library, compile and run some simple code that links against it. It could be taken from upstream's documentation / source examples.
  • If the formula is for a GUI program, try to find some function that runs as command-line only, like a format conversion, reading or displaying a config file, etc.
  • If the software cannot function without credentials, a test could be to try to connect with invalid credentials (or without credentials) and confirm that it fails as expected.
  • Same if the software requires a virtual machine, docker instance, etc. to be running.

Copy link
Contributor Author

@theory theory Apr 24, 2023

Choose a reason for hiding this comment

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

I could run ./Build test there if you don't mind the overhead.

Copy link
Member

Choose a reason for hiding this comment

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

No, it should be something a user would run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, this is more like an install test; no source here I presume. Will think about it.

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label May 15, 2023
@github-actions github-actions bot closed this May 22, 2023
@github-actions github-actions bot added the outdated PR was locked due to age label Jun 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge-skip `brew pr-automerge` will skip this pull request new formula PR adds a new formula to Homebrew/homebrew-core outdated PR was locked due to age perl Perl use is a significant feature of the PR or issue stale No recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants