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

Remove dependencies #855

Merged
merged 8 commits into from Oct 22, 2019
Merged

Remove dependencies #855

merged 8 commits into from Oct 22, 2019

Conversation

antoyo
Copy link
Member

@antoyo antoyo commented Oct 20, 2019

I'll update this text with the compilation time for every change.
It turns out git2-sys is very slow to compile (+1 minute).
Maybe we can re-enable the features of env_logger since it does not help much.

Before

Clean build: 4m 15s
Rebuild: 1m 30s

After removing docopt

Clean build: 2m 26s
Rebuild: 1m 20s

After disable features of env_logger

Clean build: 2m 23s
Rebuild: 1m 18s

After removing features of regex

Clean build: 2m 11s
Rebuild: 1m 17s

After removing git2

Clean build: 1m 28s
Rebuild: 55.24s

src/main.rs Outdated
matches.opt_str("d").as_ref().map(|string| string.as_str()),
matches.free.get(0).as_ref().map(|string| string.as_str()),
matches.free.get(1).as_ref().map(|string| string.as_str()),
matches.opt_str("o").as_ref().map(|string| string.as_str()),
Copy link
Member

@EPashkin EPashkin Oct 20, 2019

Choose a reason for hiding this comment

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

Maybe better add function for .as_ref().map(|string| string.as_str())?

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Oct 20, 2019

@antoyo Thanks, faster is better.

@GuillaumeGomez, @sdroege What you think?

@GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Oct 20, 2019

I'm the one who told him to do it. 😛

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Oct 20, 2019

By way: as I remember rust don't use libgit2 but call git from command line, maybe we can do same.

"Config file path (default: Gir.toml)",
"CONFIG",
);
options.optflag("h", "help", "Show this message");
Copy link
Member

@sdroege sdroege Oct 20, 2019

Choose a reason for hiding this comment

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

Compared to docopt or clap this is quite ugly :)

Copy link
Member

@GuillaumeGomez GuillaumeGomez Oct 20, 2019

Choose a reason for hiding this comment

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

Yep but it's just options and quite a small amount of code so who cares. :)

@sdroege
Copy link
Member

@sdroege sdroege commented Oct 20, 2019

It turns out git2-sys is very slow to compile (+1 minute).

I think it's much faster if you actually have libgit2 installed on your system :)

But yeah, works for me generally. I still think getops is rather ugly compared to alternatives but go for it if @EPashkin is fine with it too

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Oct 20, 2019

@antoyo What is time with CLI git?

@antoyo
Copy link
Member Author

@antoyo antoyo commented Oct 20, 2019

@EPashkin: I think it's a bit faster than with the previous attempt which was using libgit2 as a shared library.

@antoyo
Copy link
Member Author

@antoyo antoyo commented Oct 22, 2019

This is ready to be merged.
The other crates to remove to improve compile time will require much more work: regex and toml.

@antoyo antoyo changed the title (Do not merge) Remove dependencies Remove dependencies Oct 22, 2019
Copy link
Member

@GuillaumeGomez GuillaumeGomez left a comment

I strongly approve this PR! Awesome work, thanks @antoyo!

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Oct 22, 2019

@antoyo Thanks

@EPashkin EPashkin merged commit a2a679f into gtk-rs:master Oct 22, 2019
2 checks passed
@antoyo antoyo deleted the fix/remove-deps branch Oct 22, 2019
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.

None yet

4 participants