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

Ensure the compiler --release flag if we build with --production #372

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jwaldrip
Copy link

The documentation was misleading. So I thought it would be proper to automatically add the compiler release flag when building with the shards --production flag.

src/commands/build.cr Outdated Show resolved Hide resolved
target.main,
]
args = ["build", "-o", File.join(Shards.bin_path, target.name)]
args << "--release" if Shards.production?
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
args << "--release" if Shards.production?
args << "--release" if Shards.production? && !options.includes?("--release")

@straight-shoota
Copy link
Member

I'm not sure about this. --production affects dependency resolution, --release affects the compiler process. They don't necessarily have the same use cases. For example in testing and staging environments you might want to use --production to resolve dependencies like in production but don't bother about a release build.

@Blacksmoke16
Copy link
Member

Blacksmoke16 commented May 1, 2020

@straight-shoota It that case they can just do shards install --production. However, by using the build command with --production, IMO the intent is to create a production binary. In which case --release makes sense. I'd also argue that if you're using a binary for use in testing/staging you ought to be using it as you would in production, i.e. with --release.

I just know I fell victim to this as well, thinking it was similar to --prod flags in other langs/build tools. If anything a new flag like --strict would make more sense, where --production is a combination of --strict and --release.

@jhass
Copy link
Member

jhass commented May 1, 2020

Yes, I think the confusion/wrong expectations here are from its name. Adding --release, renaming --production to --strict or maybe --frozen sounds good to me. And then maybe we can reintroduce --production as a shorthand for the combination of those, at some point.

@asterite
Copy link
Member

asterite commented May 1, 2020

I think we should have just one flag, either --release or --production that does a combination of those two things (this PR is fine).

It's the same reason why we don't have -O1, -O2, -O3 in the compiler. We just have --release and that means "this is the build that we are going to deploy, and it better be fast". Having just one flag that means that in shards is ideal in my opinion.

I don't understand why you'd want to deploy a non-optimized build in staging. Then QA finds that stuff is slow for no reason, maybe report it... staging and production should be as similar as possible.

@j8r
Copy link
Contributor

j8r commented May 1, 2020

I agree with @straight-shoota and @Blacksmoke16 . I would also like to add that the name --production is not a very good name for a flag which installs only "main" libraries (those in the dependencies key).

It think, it would be clearer to do this way:
--no-dev: do not install any dev packages (those in in development_dependencies). Or --strict as proposed by @Blacksmoke16
--release: compile in release mode
--production: combines both --no-dev and --release

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.

7 participants