FileConfig cleanups #978

cmnrd · 2022-02-25T09:45:03Z

This PR cleans up FileConfig which currently seems to mix a variety of concerns. Despite some other simplifications and cleanups, it factors out functions that are purely related to file system access to a new static class FileUtils.

~~Note that this branch is based on #971 and hence include it's changes. The diff should clear up once #971 is merged.~~

This enables removing the dependency on context from FileConfig

edwardalee

I did not check this thoroughly, but on a spot check, all looked good to me. It's a big improvement! Except that it looks like there is a compile error with packaging Epoch... Also, this seems to be causing test failures with serialization (again, they look like compile errors).

I didn't notice the test failures. These need to be fixed before merging.

cmnrd · 2022-02-28T08:22:42Z

As far as I can see, this should be ready to be merged. I also added this PR to the 0.1.0 milestone. Given that there are no major concerns by reviewers, we should be able to merge this for 0.1.0.

lhstrh

I think it's a sensible choice to factor out all the static methods of FileConfig into a separate FileUtil class. There are some methods that ended up in CUtil, however, which seem out of place there.

lhstrh · 2022-03-01T23:29:16Z

org.lflang/src/org/lflang/generator/c/CUtil.java

+     * @param directory String representation of the director to search in.
+     * @return A Java file or null if not found
+     */
+    public static Path findFile(String fileName, Path directory) {


Why was this moved to CUtil? I don't see what's C-specific about this.

The method is only used by the C generator and the file lookup strategy seems to be very specific to the assumptions that the C generator makes.

What assumptions are those? The employed strategy seems rather generic.

lhstrh · 2022-03-01T23:29:33Z

org.lflang/src/org/lflang/generator/c/CUtil.java

+     * version do not accidentally get executed.
+     * @param fileConfig
+     */
+    public static void deleteBinFiles(FileConfig fileConfig) {


This, too, doesn't look C-specific to me.

Also this function is only used by the C-generator. Honestly, I don't quite understand what it does, but it seems to make various assumptions as to where files are placed and how they are named. Hence, I figure it will not be easily applicable to other targets. I also don't quite understand why it is needed since we have methods for cleaning up the entire bin dir.

So the function is "quarantined" here with the eventual goal of eliminating it?

Also see my comment in the main thread. I don't know the C Generator well enough to understand what this method does, why it is needed, and whether it can be eliminated. I only know that the other generators don't use it ;)

This method implements a simple functionality that all targets should implement: If a code generation or compilation fails for a program Foo.lf, then there should be no binary file bin/Foo. If there is, it is a bug. If the other targets don't implement this, then it is a bug.

cmnrd · 2022-03-02T06:59:58Z

My goal is to make FileConfig and FileUtil classes focus on one specific task each. FileConfig provides details about where certain files should be located and is meant to configure the generators. FileUtil provides generic static methods for file system access (including access to files and directories in jars or bundles). By generic FS access I mean operations like writing a file, deleting a file/directory or copying a file/directory.

The two methods in CUtil don't fit the scope of either of those classes. They are not configuration related, and they don't provide generic filesystem access. Instead, they implement very specific algorithms. These algorithms are intervened with the code generation process (delete bin in fact analysis the resource contents and deletes files where it assumes the generator has placed them, findFile makes assumptions as to where the source files for the compilation are located). Therefore, I argue that the methods should be placed in GeneratorUtils or a similar class. However, since they are only used in C and I currently don't see that they would be useful for other targets I placed them in CUtil. I might be wrong though and if people feel GeneratorUtils would be the better place, I am happy to move it over there.

lhstrh · 2022-03-02T07:26:56Z

My goal is to make FileConfig and FileUtil classes focus on one specific task each. FileConfig provides details about where certain files should be located and is meant to configure the generators. FileUtil provides generic static methods for file system access (including access to files and directories in jars or bundles). By generic FS access I mean operations like writing a file, deleting a file/directory or copying a file/directory.

Yes, I'm fully on board with this.

The two methods in CUtil don't fit the scope of either of those classes. They are not configuration related, and they don't provide generic filesystem access. Instead, they implement very specific algorithms. These algorithms are intervened with the code generation process (delete bin in fact analysis the resource contents and deletes files where it assumes the generator has placed them, findFile makes assumptions as to where the source files for the compilation are located). Therefore, I argue that the methods should be placed in GeneratorUtils or a similar class. However, since they are only used in C and I currently don't see that they would be useful for other targets I placed them in CUtil. I might be wrong though and if people feel GeneratorUtils would be the better place, I am happy to move it over there.

I understand. We can leave them in CUtil for now. I believe that the findFile method implements the policy all code generators should use for locating files that are either imported or listed in the files target property. If they don't, we should bring all targets in alignment, and if they do we have a case of code duplication. We might create an issue for this and move on?

cmnrd · 2022-03-02T07:47:16Z

I understand. We can leave them in CUtil for now. I believe that the findFile method implements the policy all code generators should use for locating files that are either imported or listed in the files target property. If they don't, we should bring all targets in alignment, and if they do we have a case of code duplication. We might create an issue for this and move on?

I think currently we don't support the files property outside the C-based targets. I am not sure how useful it would be for the other targets. For C++, it only adds redundancy but no new functionality. But yes, we can and should discuss this elsewhere.

This change reverts a small part of #978 in order to fix the broken error reporting in Epoch. closes #994

cmnrd added 2 commits February 25, 2022 10:12

Merge branch 'master' into fileconfig-cleanup

26c0a34

avoid the copy constructor in FileConfig

6020548

cmnrd requested review from Soroosh129, lhstrh and petervdonovan February 25, 2022 09:45

cmnrd added 19 commits February 25, 2022 12:40

move cleanIfNeeded to generator base

1e164d7

This enables removing the dependency on context from FileConfig

remove unused function from GeneratorBase

574dadc

move Mode enum to LFGeneratorContext (where it is used)

8f87e15

don't use fileConfig.context in code generators

58f1427

remove dependency on context from EclipseErrorReporter

dea2128

remove context from FileConfig

ce23d5b

fix test code

b7c27ca

delete unused functions

2ca537e

avoid federation specific code in FileConfig

462e062

move C-specific function deleteBinFiles() to CUtil

f5e6d32

bugfix

d93657e

Create new class FileUtil and move some methods from FileConfig

3610b66

make various copy methods static

b972a6d

move all copy methods to FileUtil

8b00182

copyUserFiles is only ever used in the CGenerator...

1bc3210

move C specific FileUtil functions to CUtil

3dd7033

move getIResource methods to FileUtil

e246f92

remove redundant getName()

4313e3f

getSubPkgPath should not be static

4e01e55

cmnrd marked this pull request as ready for review February 25, 2022 16:14

edwardalee previously approved these changes Feb 25, 2022

View reviewed changes

cmnrd added 3 commits February 25, 2022 18:07

fix diagrams

7ae443e

files is only used in C, CCpp and Python

7609663

automatically add the src directory to the C++ includes

51e0aad

cmnrd added 7 commits February 25, 2022 18:50

copy user files before invoking proto-c

98f8abb

uncomment copyUserFiles

7f30061

revert an accidental change

21ab9a9

Merge branch 'master' into fileconfig-cleanup

18a968e

port changes to the Python generator

6d9168a

move methods for file writing to FileUtil

ce4b600

rename JavaGeneratorUtils -> GeneratorUtils

a7e8f50

cmnrd added this to the 0.1.0 milestone Feb 28, 2022

Merge branch 'master' into fileconfig-cleanup

6e40eea

fix compilation error in tests

b12df01

cmnrd mentioned this pull request Feb 28, 2022

Enable generation of ROS2 nodes directly from C++ code #984

Merged

cmnrd requested a review from edwardalee March 1, 2022 18:06

lhstrh reviewed Mar 1, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into fileconfig-cleanup

18df2e7

lhstrh approved these changes Mar 2, 2022

View reviewed changes

lhstrh mentioned this pull request Mar 2, 2022

Establish uniformity across targets in the way files are looked up #992

Open

cmnrd merged commit c2633ee into master Mar 2, 2022

cmnrd deleted the fileconfig-cleanup branch March 2, 2022 08:18

cmnrd added a commit that referenced this pull request Mar 3, 2022

Fix eclipse error reporting

3a1ca2e

This change reverts a small part of #978 in order to fix the broken error reporting in Epoch. closes #994

cmnrd mentioned this pull request Mar 3, 2022

Fix eclipse error reporting #995

Merged

cmnrd added a commit that referenced this pull request Mar 3, 2022

Fix eclipse error reporting

acfad28

This change reverts a small part of #978 in order to fix the broken error reporting in Epoch. closes #994

lhstrh mentioned this pull request Mar 17, 2022

Create an automated release workflow #338

Closed

lhstrh added the refactoring Code quality enhancement label Mar 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FileConfig cleanups #978

FileConfig cleanups #978

cmnrd commented Feb 25, 2022 •

edited

Loading

edwardalee left a comment •

edited

Loading

cmnrd commented Feb 28, 2022

lhstrh left a comment

lhstrh Mar 1, 2022

cmnrd Mar 2, 2022

lhstrh Mar 2, 2022

lhstrh Mar 1, 2022

cmnrd Mar 2, 2022

lhstrh Mar 2, 2022

cmnrd Mar 2, 2022

edwardalee Mar 2, 2022

cmnrd commented Mar 2, 2022

lhstrh commented Mar 2, 2022

cmnrd commented Mar 2, 2022

FileConfig cleanups #978

FileConfig cleanups #978

Conversation

cmnrd commented Feb 25, 2022 • edited Loading

edwardalee left a comment • edited Loading

Choose a reason for hiding this comment

cmnrd commented Feb 28, 2022

lhstrh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnrd commented Mar 2, 2022

lhstrh commented Mar 2, 2022

cmnrd commented Mar 2, 2022

cmnrd commented Feb 25, 2022 •

edited

Loading

edwardalee left a comment •

edited

Loading