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

miranda 2.066 (new formula) #51003

Closed
wants to merge 4 commits into from
Closed

miranda 2.066 (new formula) #51003

wants to merge 4 commits into from

Conversation

@vladimyr
Copy link
Contributor

vladimyr commented Mar 2, 2020

  • Have you followed the guidelines for contributing?
  • 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 <formula>)?

I'm aware that Miranda isn't that popular anymore but it had a strong influence on the Haskell back in the day and after years of being closed source, it went completely open-source recently. I thought it would be cool to add it here so people can install it more easily and give it a try.

/cc @MiroDojkic

@Bo98 Bo98 added the new formula label Mar 2, 2020
@Bo98

This comment has been minimized.

Copy link
Member

Bo98 commented Mar 2, 2020

@BrewTestBot test this please

@vladimyr

This comment has been minimized.

Copy link
Contributor Author

vladimyr commented Mar 2, 2020

@Bo98 I figured out why this fails. Miranda compiles stdlib ($(LIB)/miralib/stdenv.m) as part of the installation. Produced binary file ($(LIB)/miralib/stdenv.x) contains the creation date of its source ($(LIB)/miralib/stdenv.m). Once you run Miranda it will check that those dates match. It works locally but once you put them in the bottle and change all timestamps (https://github.com/Homebrew/brew/blob/10ba0d5/Library/Homebrew/dev-cmd/bottle.rb#L282-L291) they do not match anymore and the only way to fix it is to remove stdenv.x and compile it again so it contains updated creation date of stdenv.m. Now I'm not sure how to resolve it 🤔

Why does brew bottle alter timestamps anyway? Also is there a way to add some kind of post-install step which would instruct Miranda to recompile stdlib and other stuff that is needed?

EDIT: Figured out how to recompile stuff in postinstall hook 🎉

vladimyr added 2 commits Mar 2, 2020
@vladimyr vladimyr force-pushed the vladimyr:miranda branch from 6f9d20e to 2ab172e Mar 2, 2020
Copy link
Member

SMillerDev left a comment

Please patch the source upstream instead of drastically changing the install in the formula.

# Compile `stdenv.m` & provided examples
system "#{bin}/mira", "-make", "-lib", miralib, "#{miralib}/**/*.m"
Comment on lines +50 to +51

This comment has been minimized.

Copy link
@SMillerDev

SMillerDev Mar 8, 2020

Member

Why do these need to be compiled here?

This comment has been minimized.

Copy link
@vladimyr

vladimyr Mar 8, 2020

Author Contributor

Strictly speaking, examples do not need to be precompiled but miralib aka stdenv.m needs to be compiled AOT because it is implicitly imported in any script you are trying to execute. I've excluded compilation from an original makefile by removing exfiles rule due to the issue described in my previous comment: #51003 (comment)
Which is why I re-introduced it as part of the post-install script.

# Makefile assumes that provided paths exist
bin.mkpath
lib.mkpath
man1.mkpath
Comment on lines +14 to +17

This comment has been minimized.

Copy link
@SMillerDev

SMillerDev Mar 8, 2020

Member

Can you provide a patch upstream to fix this?

inreplace "Makefile" do |s|
# Skip compilation of `stdenv.m` & provided examples
s.gsub! " exfiles", ""
# Do NOT touch `#{lib}/miralib/` permissions
s.gsub! "./protect", "/usr/bin/true"
s.gsub! "./unprotect", "/usr/bin/true"
# Do NOT change `#{lib}/miralib/` ownership
s.gsub! "./ugroot", "whoami"
end
Comment on lines +29 to +37

This comment has been minimized.

Copy link
@SMillerDev

SMillerDev Mar 8, 2020

Member

Please provide a patch upstream to fix this.

# Remove compiled files, if any
rm_f "#{miralib}/preludx"
rm Dir["#{miralib}/**/*.x"]
Comment on lines +42 to +44

This comment has been minimized.

Copy link
@SMillerDev

SMillerDev Mar 8, 2020

Member

Why is this needed?

This comment has been minimized.

Copy link
@vladimyr

vladimyr Mar 8, 2020

Author Contributor

To reassure no compiled files have been put inside the bottle. It should be sufficient to simply call make cleanup, which is already done, but I'm using this an additional safety measure. Again, if you bottle those while altering creation timestamps on accompanying sources you'll get runtime error on the first run of Miranda environment.

This comment has been minimized.

Copy link
@MikeMcQuaid

MikeMcQuaid Mar 9, 2020

Member

Why can't compiled file be put inside the bottle?

This comment has been minimized.

Copy link
@vladimyr

vladimyr Mar 9, 2020

Author Contributor

Because of #51003 (comment)

@SMillerDev

This comment has been minimized.

Copy link
Member

SMillerDev commented Mar 8, 2020

@Homebrew/core any opinions on this compilation on the user machine?

@Bo98

This comment has been minimized.

Copy link
Member

Bo98 commented Mar 8, 2020

I suppose my question would be: how tasking is it? How long does it take?

@vladimyr

This comment has been minimized.

Copy link
Contributor Author

vladimyr commented Mar 8, 2020

Please patch the source upstream instead of drastically changing the install in the formula.

I'm not sure that is even possible... 🤔

I've used the following repo: https://github.com/rljacobson/Miranda/tree/49d6dcc in response to your comments because it contains verbatim import of source code dump that is published on Miranda download page: http://miranda.org.uk/downloads Note that there isn't an actively developed upstream out there and I'm using tarball as a source for this formula. (There are some signs of a continuation of the development under new leadership: https://github.com/ncihnegn/miranda but this is currently a one-man show and I'm not really in favor of using it as upstream/base of the submitted formula atm.)

Miranda wasn't open source until 2020-31-01 and code that has been released was last changed decades ago excluding necessary tweaks to get it to compile using present time compilers. According to bundled readme I could theoretically send a patch to mira-bugs (at) miranda.org.uk but I don't think it will end up accepted.
There are two issues with bundled makefile:

  1. Assuming the existence of BIN, LIB and MAN paths which is fairly easy to fix and will probably end up being accepted 🤔
  2. Tweaking *nix permissions and ownership of produced artifacts which does not go well with brew's way of doing things. I'm not really optimistic that Mr. Turner is willing to alter his makefile just to conform to brew's conventions given in mind his ages-old software become vaporware a long time ago. 🤷‍♂

This is a sort of chicken and egg problem. I'm aware of the ugliness of the current solution but I'm proposing it in order to make Miranda (in its current state) more accessible to the general public. Hopefully, that will result in a continuation of development after which upstream should get switched and formula should be cleared out of present hacks.

@vladimyr

This comment has been minimized.

Copy link
Contributor Author

vladimyr commented Mar 8, 2020

@Homebrew/core any opinions on this compilation on the user machine?

Compilation of Miranda's stdenv?

PS Sorry, wrong button 🤦‍♂

@vladimyr vladimyr closed this Mar 8, 2020
@vladimyr vladimyr reopened this Mar 8, 2020
@vladimyr

This comment has been minimized.

Copy link
Contributor Author

vladimyr commented Mar 8, 2020

Here are benchmark results made on my machine:

image

It ends up around 50ms together with removing stuff from previous compilation so real-world numbers would be even lower.

depends_on "byacc" => :build

def install
# Make tasks are required to run sequentially

This comment has been minimized.

Copy link
@MikeMcQuaid

MikeMcQuaid Mar 9, 2020

Member

Why? Can an issue be submitted upstream for this?

This comment has been minimized.

Copy link
@vladimyr

vladimyr Mar 9, 2020

Author Contributor

As I explained earlier upstream in the conventional sense does not exist and this is built upon source code dump that hasn't been actively maintained for a long time.

This comment has been minimized.

Copy link
@MikeMcQuaid

MikeMcQuaid Mar 9, 2020

Member

If there is no upstream this software is not maintained and not a good fit for Homebrew/core, sorry (https://docs.brew.sh/Acceptable-Formulae#niche-or-self-submitted-stuff)

This comment has been minimized.

Copy link
@vladimyr

vladimyr Mar 9, 2020

Author Contributor

Just to make it super clear:

this is built upon source code dump that hasn't been actively maintained for a long time.

It is my assumption. I can't and do not know for sure due to it being closed source until recently.

Maybe the author still occasionally works on it and accepts patches but I'm not willing to start contributing by mailing patches around without any transparency.

Again, I just wanted to make it easier for people to play with it but if this is a bad place to host it I'll move it to personal tap. I'm keeping it open and waiting for your final decision.

This comment has been minimized.

Copy link
@MikeMcQuaid

MikeMcQuaid Mar 9, 2020

Member

Ok, will pass on this for now given the above, thanks anyway!


miralib = "#{lib}/miralib"

args = %W[

This comment has been minimized.

Copy link
@MikeMcQuaid

MikeMcQuaid Mar 9, 2020

Member

Please inline this given it's not dynamic.

]

inreplace "Makefile" do |s|
# Skip compilation of `stdenv.m` & provided examples

This comment has been minimized.

Copy link
@MikeMcQuaid

This comment has been minimized.

Copy link
@vladimyr

vladimyr Mar 9, 2020

Author Contributor

Because it will end up bottled and ultimately cause failures on the first run due to #51003 (comment)

inreplace "Makefile" do |s|
# Skip compilation of `stdenv.m` & provided examples
s.gsub! " exfiles", ""
# Do NOT touch `#{lib}/miralib/` permissions

This comment has been minimized.

Copy link
@MikeMcQuaid

This comment has been minimized.

Copy link
@vladimyr

vladimyr Mar 9, 2020

Author Contributor

Because it alters them in a way subsequent brew remove miranda attempt fails with insufficient rights.

s.gsub! "./ugroot", "whoami"
end

system "make", "cleanup"

This comment has been minimized.

Copy link
@MikeMcQuaid

MikeMcQuaid Mar 9, 2020

Member

Why is cleanup needed before install?

This comment has been minimized.

Copy link
@vladimyr

vladimyr Mar 9, 2020

Author Contributor

Because tarball, unfortunately, contains some precompiled assets and this clears them out.

# Remove compiled files, if any
rm_f "#{miralib}/preludx"
rm Dir["#{miralib}/**/*.x"]

This comment has been minimized.

Copy link
@MikeMcQuaid

MikeMcQuaid Mar 9, 2020

Member

Why can't compiled file be put inside the bottle?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Mar 9, 2020

Why does brew bottle alter timestamps anyway?

To provide a degree of reproducibility (https://reproducible-builds.org).


I think we need to find a way to not recompile things in post_install. Homebrew is a binary package manager so these sort of hacks unfortunately aren't acceptable in Homebrew/core.

Thanks for you work so far!

@MikeMcQuaid MikeMcQuaid closed this Mar 9, 2020
@vladimyr

This comment has been minimized.

Copy link
Contributor Author

vladimyr commented Mar 9, 2020

FWIW I've managed to remove postinstall step from the formula: https://github.com/vladimyr/homebrew-core/blob/b610b25/Formula/miranda.rb

@FranklinChen

This comment has been minimized.

Copy link
Contributor

FranklinChen commented Mar 24, 2020

Has someone contacted the author of Miranda, David Turner, to resolve these build issues?

@vladimyr

This comment has been minimized.

Copy link
Contributor Author

vladimyr commented Mar 24, 2020

I haven't because these aren't really issues, they are rather consequences of different approaches. Asking to conform to brew's way of doing things didn't sound promising to me in a given context but feel free to try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.