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

Drop the rebuild feature of lfc #530

Merged
merged 21 commits into from
Oct 19, 2021
Merged

Drop the rebuild feature of lfc #530

merged 21 commits into from
Oct 19, 2021

Conversation

cmnrd
Copy link
Collaborator

@cmnrd cmnrd commented Sep 23, 2021

The automatic rebuild feature of lfc (lfc -r) is convenient for development, but can easily cause trouble (see #521 and #524). I argue that we should drop this feature and let gradle handle the rebuilding for us (see #528). There is already a runLfc task which provides the same functionality as lfc -r. This PR completely removes the rebuild feature of lfc and slightly improves usability of the runLfc task, by setting the default working directory to the root of the LF project (instead of org.lflang.lfc as before).

Copy link
Contributor

@Soroosh129 Soroosh129 left a comment

Choose a reason for hiding this comment

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

This looks good to me. This change cleans up Main.java considerably.

@lhstrh
Copy link
Member

lhstrh commented Sep 23, 2021

This isn't so much of a clean up as it is a feature removal, and it happens to concern a feature that I use very frequently. One of the motivations behind this code was to simplify the lfc script, in part to ease porting it to power shell, to avoid code replication, and to take advantage of nice library code for printing menus.

I'm OK with these changes, but I would like to keep the -r feature as an undocumented flag that only works in a development setup, because I have it in muscle memory now, and it will be a nuisance to switch to using gradle commands to achieve the same thing. I think this is best done in the lfc script. I can take a stab at implementing this (in the bash script, not the ps1 script) and I'll push to the same branch so we can evaluate it as part of the this PR.

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

I agree that the Java-based rebuild functionality was:

  1. too complicated;
  2. too brittle; and
  3. not suitable for non-development use.

I've instead worked rebuild functionality into the Bash script, which is tiny compared to the amount of Java code that was slashed. This, however, reminded me of another issue, which is that we do some work on the scripts as part of the release script, which probably has to be looked at (I expect this might break the release). Ideally, we just work some conditionals into our scripts so that no adaptations are necessary for releases.

@cmnrd
Copy link
Collaborator Author

cmnrd commented Sep 24, 2021

Currently, the benchmark tests fail because the bash script detects --runtime-version as -r. The benchmark runner uses this flag to tell lfc which version of the C++ runtime it should use.

Honestly, I think that lfc should really just be a launcher. It shouldn't be able to build or rebuild the jars. This dual use of the lfc script for launching and building is the main reason why we have to do the brittle patching in our nightly builds. If it would just be a launcher, there would also be no need for patching.

I understand that lfc -r is in your muscle memory, but it also quite complicates things in our scripts. I think a good alternative could be to make the build-lfc script more powerful. Maybe build-lfc -r could build and run lfc?

@lhstrh
Copy link
Member

lhstrh commented Sep 24, 2021

@cmnrd: You might be right. I like your suggestion adding the capability to also run stuff using build-lfc. In any case, I agree that the patching that's currently being done for the nightly build has to go. I'll push a fix.

@cmnrd
Copy link
Collaborator Author

cmnrd commented Sep 27, 2021

I worked on the lfc script, and I think I was able to make it much more robust. With the new design, the script checks first if it is in a developer setup (i.e. are the sources available) or in a pre-built package. Depending on this, it looks up the appropriate jar. The version of the jar is not fixed anymore. In case the script finds multiple options, it uses the latest version. If we are in a developer setup and there is no jar (or a rebuild is requested with -r), then the jar is build with gradle.

These changes also remove the patching from our packaging script. Thus, this closes #434.

With these changes, I also convinced myself that it is possible to have an lfc script that supports the rebuild flag and that works in a developer and a packaged context without modification. I am unsure what to do now. We can go for the rebuild feature in lfc, but then the build-lfc script is redundant and I would delete it. Or we use lfc purely for running the standalone compiler and extend the build-lfc script such that it can optionally also run lfc. What do you think?

Note that the Windows lfc.ps1 script is not updated yet.

@Soroosh129
Copy link
Contributor

Soroosh129 commented Sep 28, 2021

I have a question: Why can't lfc -r just use build-lfc to avoid duplicate logic? They are after all in the same bin directory in a developer setup, and build-lfc is non-existent in a non-developer setup. Perhaps only the existence of build-lfc can be checked?

@codecov
Copy link

codecov bot commented Sep 28, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@249218d). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #530   +/-   ##
=========================================
  Coverage          ?   66.36%           
  Complexity        ?     3462           
=========================================
  Files             ?      132           
  Lines             ?    22294           
  Branches          ?     2883           
=========================================
  Hits              ?    14795           
  Misses            ?     6352           
  Partials          ?     1147           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 249218d...1d18502. Read the comment docs.

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

I've followed your suggestion, @cmnrd, and moved the -r flag to build-lfc.

fi

# Check if jar is missing or out-of-date compared to the sources
if [ ! -f "${lfc_jar_snapshot_path}" ] || ! "${FIND}" "${lfbase}/src" -path "${lfbase}/test" -prune -o -type f -newer "${lfc_jar_snapshot_path}" -exec false {} +; then
Copy link
Member

Choose a reason for hiding this comment

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

@cmnrd: I'm a little unsure about the "${lfbase}/test" in here. Why should we rebuild if a test has changed?

This came from the original lfc-build script...

@lhstrh
Copy link
Member

lhstrh commented Oct 19, 2021

I just realized the the ps1 script still has to be updated though. But considering how simple lfc has become, this shouldn't be super difficult to fix...

@lhstrh lhstrh merged commit 84efdc8 into master Oct 19, 2021
@lhstrh lhstrh deleted the lfc-rebuild branch October 19, 2021 22:07
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.

4 participants